Skip to content
Snippets Groups Projects

WIP: Import unreadable files

Closed Jim MacArthur requested to merge jmac/vdir_import_unreadable_files into master
1 unresolved thread

Description

Certain artifacts stage things which have no read permission. See #684 for an example.

Changes proposed in this merge request:

  • Alter _process_list in utils.py so it can read files with no read permission.

There were other previously proposed changes which aren't necessary:

  • Alter _process_list so it can do the same job for directories, symlinks and other node types

    • Directories, devices, and fifos are created at the destination without needing to read the source.
    • The permissions set on symlinks are irrelevant under a unix-based OS, and do not need to have their contents read anyway (as above)
  • Alter cascache.py so it can hash unreadable files

  • Allow CasBasedDirectory to import unreadable files

    • #429 is expected to correct CASCache so it can hash unreadable files. CasBasedDirectory doesn't need to open any files; it just asks the CASCache to hash them with add_object.

This merge request, when approved, will close issue/bug: #684


Edited by Jim MacArthur

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
831 831 result.ignored.append(path)
832 832 continue
833 833
834 if not file_stat.st_mode & stat.S_IRUSR:
835 os.chmod(srcpath, file_stat.st_mode | stat.S_IRUSR)
836
834 837 actionfunc(srcpath, destpath, result=result)
  • Not really happy about changing the source permissions here. But I don't have any other ideas at the moment.

  • I agree, especially having to do it in such a generic utility function. Also, based on my analysis and testing, this doesn't even fix the exceptions reported in #684.

    The permission error is triggered by the fallback of shutil.move because the destination directory for staging sources is typically no longer on the same filesystem (~/.cache vs. /tmp). The fallback of shutil.move is not related to BuildStream's _process_list utility function, so patching that up won't fix anything.

    I propose that we change the temporary directory in Element._stage_sources_at to be within ~/.cache/buildstream as it used to be. This will provide a massive speedup (shutil.move can use os.rename instead of having to copy the whole tree) and fix the exceptions reported in #684.

    However, I expect that with this temporary directory change we will still trigger an exception in import_files that this MR would fix. But even if we applied this change, this would still leave #429.

    I'm tempted to say that it's the source's responsibility to stage readable files and fix up source plugins that don't always do that (I suspect ostree and tar, the others might be ok). How do others feel about this?

  • Please register or sign in to reply
  • !856 (merged) moved the temporary storage to ~/.cache.

  • closed

  • Please register or sign in to reply
    Loading