Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(tar): append slash to top-level directory mtree entries #852

Merged

Conversation

sin-ack
Copy link
Contributor

@sin-ack sin-ack commented May 23, 2024

bsdtar's mtree format has a quirk wherein entries without "/" in their first word are treated as "relative" entries, and "relative" directories will cause tar to "change directory" into the declared directory entry. If such a directory is followed by a "relative" entry, then the file will be created within the directory, instead of at top-level as expected. To mitigate, we append a slash to top-level directory entries.

Fixes #851.


Changes are visible to end-users: no

Test plan

  • New test cases added: //:tar_test13, //lib/tests/tar:test14

@CLAassistant
Copy link

CLAassistant commented May 23, 2024

CLA assistant check
All committers have signed the CLA.

@alexeagle
Copy link
Collaborator

Thanks - you should be able to find somewhere in https://github.com/aspect-build/bazel-lib/blob/main/lib/tests/tar/BUILD.bazel that tickles this case, and assert that the new mtree matches some string

@sin-ack
Copy link
Contributor Author

sin-ack commented May 24, 2024

I tried to introduce a test, however the main problem is that any file I create with file_write is created inside lib/tests/tar/, and mtree_spec doesn't hit the bug. That also made me realize that we need to add this logic to mtree_mutate (because of strip_prefix), but that doesn't solve the problem that we can't test mtree_spec by itself. What should I do here?

@alexeagle
Copy link
Collaborator

Simple answer would be to add the test case to the root package in this repo with a comment about why we're doing something unusual

@sin-ack sin-ack force-pushed the tar-mtree-avoid-relative-entry branch 2 times, most recently from 0636e4f to 76bf44c Compare May 27, 2024 08:32
@sin-ack
Copy link
Contributor Author

sin-ack commented May 27, 2024

Changes since last push:

  • mtree_mutate now also correctly handles directory entries with a single component when strip_prefix is given
  • Added tests for mtree_spec and mtree_mutate

@sin-ack
Copy link
Contributor Author

sin-ack commented Jun 5, 2024

Could I get another look at this please? :)

@alexeagle
Copy link
Collaborator

Sorry the CI doesn't trigger for you automatically. Looks like there are some errors, can you reproduce them?

@sin-ack
Copy link
Contributor Author

sin-ack commented Jun 6, 2024

Interesting, it seems like the output order of mtree_spec is either random or differs between my machine and the CI. Perhaps I could write a genrule that orders the output of mtree in such a way to trigger the bug.

bsdtar's mtree format has a quirk wherein entries without "/" in their
first word are treated as "relative" entries, and "relative" directories
will cause tar to "change directory" into the declared directory entry.
If such a directory is followed by a "relative" entry, then the file
will be created within the directory, instead of at top-level as
expected. To mitigate, we append a slash to top-level directory entries.

Fixes bazel-contrib#851.
@sin-ack sin-ack force-pushed the tar-mtree-avoid-relative-entry branch from 76bf44c to 667f816 Compare June 19, 2024 08:52
@sin-ack
Copy link
Contributor Author

sin-ack commented Jun 19, 2024

Sorry for the delay, I was on vacation. I've made the tests run the sort command on the output, which should hopefully make the order consistent between systems now.

@alexeagle Could I get a re-run of the tests please?

@alexeagle alexeagle merged commit cc956d8 into bazel-contrib:main Jul 2, 2024
24 checks passed
@sin-ack
Copy link
Contributor Author

sin-ack commented Jul 3, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: mtree_spec generates incorrect spec file if the root directory has a file and a directory
4 participants