-
Notifications
You must be signed in to change notification settings - Fork 179
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
Ensure the experimental/ tree lands in the output release archive #142
Conversation
pkg/distro/BUILD
Outdated
@@ -19,7 +19,7 @@ pkg_tar( | |||
mode = "0444", | |||
# Make it owned by root so it does not have the uid of the CI robot. | |||
owner = "0.0", | |||
package_dir = ".", | |||
package_dir = "pkg", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes the location of the rules. Everyone using it would have to change from @rules_pkg//:pkg.bzl to @rules_pkg//pkg:pkg.bzl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that was the intent of the original issue.
Even if this code is not desired, the rest of it is needed to ensure that the experimental RPM generator lands in the release tarball. Let me clean this up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... I've been thinking yesterday that this is something we might want to do anyway.
How about we discuss an optimal arrangement for the code first, and then move to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is, move most of the files from pkg/.bzl to pkg/pkg/.bzl, but leave distro and tests at the same level. Now the distribution tarball is only pkg/... experimental/... and the top level BUILD and license. We can put in a forwarder for a while so code using pkg:pkg.bzl will still work during migration.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dannysullivan because that move would impact docker's near term plan to point at archive.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is, move most of the files from pkg/.bzl to pkg/pkg/.bzl, but leave distro and tests at the same level. Now the distribution tarball is only pkg/... experimental/... and the top level BUILD and license. We can put in a forwarder for a while so code using pkg:pkg.bzl will still work during migration.
WDYT?
Makes sense to me. We may even additionally want to move to the model of:
pkg/defs.bzl
provides forwarders for all non-experimental rulespkg/internal/*.bzl
provides rule implementations
Not sure where the python and other scripts would go. Same directory? experimental/ could follow the same pattern.
In the meantime, I removed the line in question and included all the files needed to run the experimental rpm packager.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of internal. It makes the code harder to trace and, without enforced visibility, it does not ensure that people won't depend on it anyway.
The `experimental/` tree containing the then `pkgfilegroup` rule was missing from the 0.2.5 release archive. This change ensures that the files in the experimental/ tree are included in release packages so users can use the rules within. Testing was done by trying to run the experimental `pkg_rpm` rule on a dummy project importing rules_pkg.
1e5d153
to
95bb3da
Compare
The
experimental/
tree containing the thenpkgfilegroup
(and nowpkg_filegroup
) rule was missing from the 0.2.5 release archive.This change ensures that the files in the experimental/ tree are included in release packages so users can use the rules within.
Testing was done by trying to run the experimental
pkg_rpm
rule on a dummy project importing rules_pkg.