Skip to content

pkg_install: Support TreeArtifacts.#885

Merged
aiuto merged 3 commits intobazelbuild:mainfrom
jacky8hyf:main
Aug 28, 2024
Merged

pkg_install: Support TreeArtifacts.#885
aiuto merged 3 commits intobazelbuild:mainfrom
jacky8hyf:main

Conversation

@jacky8hyf
Copy link
Contributor

Implementation notes:
For tree artifacts, when creating directories, we mostly follow the modes set for the whole TreeArtifact, but also +x to allow searching the directory. This is similar to how pkg_tar etc. handles things.

Link: #308

They are so confusing; I would've thought that because they
took the parameters, they would set the user/group for me.
But in fact, _chown_chmod needs to be called afterwards.
Implementation notes:
For tree artifacts, when creating directories, we mostly follow the
modes set for the whole TreeArtifact, but also +x to allow searching
the directory. This is similar to how pkg_tar etc. handles things.

Link: bazelbuild#308
@jacky8hyf
Copy link
Contributor Author

I did a few changes:

  • Clean up some internal functions (done in a separate patch and rebased the patch on top of it)
  • Added tests
  • Addressed the comments.

As such, I did a force push for a fresh review. Please take a look; thanks!

I didn't handle symlinks within the tree artifact, though.

@jacky8hyf
Copy link
Contributor Author

Hello, any updates? Could you please take a look at this pull request? Thanks!

@cgrindel cgrindel requested a review from aiuto August 28, 2024 17:56
Copy link
Collaborator

@cgrindel cgrindel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I will let @aiuto look before merging.

Copy link
Collaborator

@aiuto aiuto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this one.

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.

3 participants