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

Add support for setting uid/gid from pkg_attributes #671

Merged
merged 5 commits into from
May 1, 2023

Conversation

AustinSchuh
Copy link
Contributor

This enables the user to produce identical tarballs to fix #670 by adding a uid/gid option to pkg_attributes.

Copy link
Collaborator

@nacl nacl 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 the patch! Below are things I noticed, but unfortunately the manifest format changed recently, so there's a number of merge conflicts.

Some things I wonder about outside of the motivating use case (discussion in #670):

  • What about pkg_rpm? It's fine not to include it, as it might be a little complicated and/or not possible.
  • Perhaps uid/gid should not be named in pkg_attributes? An alternative interface design would be to detect if user/group are ints, and then convert that to uid/gid on the backend. WDYT?

pkg/mappings.bzl Show resolved Hide resolved
pkg/mappings.bzl Outdated Show resolved Hide resolved
pkg/mappings.bzl Outdated Show resolved Hide resolved
pkg/private/pkg_files.bzl Show resolved Hide resolved
pkg/private/pkg_files.bzl Outdated Show resolved Hide resolved
This enables the user to produce identical tarballs to fix
bazelbuild#670

Signed-off-by: Austin Schuh <[email protected]>
@AustinSchuh
Copy link
Contributor Author

Ok, I think this now works again. I don't know how to run the RPM tests. They appear to be disabled locally. If you can give me a hint, I can see what I can do.

pkg/mappings.bzl Show resolved Hide resolved
@aiuto aiuto requested review from nacl and removed request for jylinv0 February 28, 2023 03:04
@aiuto
Copy link
Collaborator

aiuto commented Mar 1, 2023

@nacl I think it is blocked on you resolving a comment.

@AustinSchuh
Copy link
Contributor Author

Ping, @nacl , any additional feedback?

Copy link
Collaborator

@nacl nacl left a comment

Choose a reason for hiding this comment

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

Sorry for the delays on this. I got a bit stuck on some internal stuff and only recently got a chance to refocus.

This seems fine, but I think the docs could use a slight adjustment. See below WRT the RPM tests.

Ok, I think this now works again. I don't know how to run the RPM tests. They appear to be disabled locally. If you can give me a hint, I can see what I can do.

The RPM tests require external RPM tooling (rpm and rpmbuild). On macOS, you can get a recent copy of these from homebrew. On Linux, you can use your local package manager. Either should work.

Unfortunately, RPM packaging is tricky and a portable toolchain setup is difficult to make.

LMK if you have any further issues. If you're having issues with this, the documentation can be updated and this investigation can be added to the backlog. Intuitively, it may not make sense for RPMs (and Debs) to support numeric ids.

docs/latest.md Show resolved Hide resolved
pkg/mappings.bzl Show resolved Hide resolved
@aiuto
Copy link
Collaborator

aiuto commented Mar 24, 2023

Friendly ping. This should get merged, can we come to closure on the last few nits?

@nacl nacl merged commit e7a1ba1 into bazelbuild:main May 1, 2023
@AustinSchuh
Copy link
Contributor Author

Sorry for the long delay... I went from having time to not having time really fast.

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.

pkg_files and pkg_tar produce different tarballs.
3 participants