Review public `Directory` API

[BuildStream-Migration-Bot]

See original issue on GitLab In GitLab by [Gitlab user @juergbi] on Apr 27, 2020, 07:58

We should make sure we're happy to support the public Directory API long term before releasing 2.0. Aspects to consider:

• Do we want to keep the *paths: str approach (variable argument list for individual path components)? It can be convenient to avoid the need for string joining/splitting and eliminates the issue of OS-specific path separators. However, it's different from prevalent filesystem APIs, which may confuse plugin developers. And there are also cases where the API is less convenient (e.g., using paths specified in .bst files). If we decide to switch to a traditional path-based API, I'd always use / as path separator to match REAPI and avoid issues building e.g. Linux artifacts from Windows (using remote execution or Docker).
• import_files(): Do we need the full complexity of the current API? filter_callback, report_written, FileListResult
• import_single_file(): Do we want to support specifying a destination path? Maybe rename the method to import_file()
• Are list_modified_paths() and mark_unmodified() the best approach for what compose needs?
• Is set_deterministic_user() needed in the public API?
• Directory.stat() returns a os.stat_result. Would it be better to define a custom, extensible data type instead? https://gitlab.com/BuildStream/buildstream/-/merge_requests/1878#note_331809335
• Get rid of the undocumented dictionary returned from the CasBasedDirectory.walk() API
  • This is used in the frontend in order to implement bst artifact list-contents
  • To avoid redundancy and duplication, it would be suitable to use the same objects which Directory.stat() returns in this walk() API, perhaps a custom object with a bit more context than os.stat_result would be beneficial here, as we can add more context about the file, such as symbolic link targets.
  • Doing this part should result in better updated documentation of what is going on in the _frontend/cli.py and _frontend/widget.py portions which deal with formatting this data for the user.
• Consider removing the mark_unmodified() API, as this sets the deterministic mtime only for the local filebased directory, but this is only relevant for builds and we only ever stage into CAS now
  • Note: deterministic mtime is now handled by not specifying any mtime in cas, and expecting a sandbox backend which stages files from CAS, like buildbox to use the deterministic mtime in place of any explicitly set mtime.

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @juergbi] on Apr 27, 2020, 09:29

changed the description

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @juergbi] on Apr 27, 2020, 09:29

mentioned in merge request !1878

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @tristanvb] on Jun 24, 2020, 11:16

Worth linking here, is my motion to remove APIs which allow plugins to exploit the Sandbox by writing content directly into it.

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @tristanvb] on Jun 24, 2020, 11:19

Are list_modified_paths() and mark_unmodified() the best approach for what compose needs?

I would say that:

• It can be interesting to offer something along these lines in public API
• We should not block 2.0 on this, instead:
  • We can make this private API
  • The compose element anyway remains in core where it can access private API

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @tristanvb] on Jun 24, 2020, 11:20

Is set_deterministic_user() needed in the public API?

I don't think so, and was surprised to see it in the Directory API.

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @tristanvb] on Aug 7, 2020, 13:03

changed the description

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @abderrahimk] on Aug 7, 2020, 22:21

mentioned in merge request !2018

[BuildStream-Migration-Bot]

In GitLab by [Gitlab user @tristanvb] on Aug 13, 2020, 09:26

changed the description