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

Provide capability to rename archives based on configuration values #198

Merged
merged 30 commits into from
Sep 30, 2020

Conversation

aiuto
Copy link
Collaborator

@aiuto aiuto commented Jul 9, 2020

This is to address #101 #193 and maybe help with #143

The goal is to allow all packaging rules to precisely name their output files in a way that is consistent with the naming conventions of the scheme, even if that requires pieces of the file name which can not be known until build time, such as cpu type.

The basic method is:

  • allow all rules to specify the name of the output archive with package_file_name, rather than using a default
  • create a PackageVarialbesInfo provider which specifies a map of values to replacements (e.g. {cpu:x86_64, copt:debug})
  • if apkg_ rule target adds a package_variables attribute (which must provide PackageVariablesInfo), the provided dictionary will be used for substitutions in package_file_name
  • pkg_ rules will also return a PackageArtifactInfo provider which will map target label to the file name actually generated. This is need in some situations when associated rules must know the exact name of the final package.

If a target does not use package_file_name then the out attribute is created as before (package + "." + extension).
If it does, then out will be a symlink to the name specified by package_name.

This PR only deals with pkg_tar. The other package types, and substitution into the spec files will come in followup changes.

@aiuto aiuto added the WIP label Jul 9, 2020
@aiuto aiuto requested review from vmax, dannysullivan and nacl July 9, 2020 19:22
@aiuto
Copy link
Collaborator Author

aiuto commented Jul 9, 2020

cc: @davschne @konste @KaoruDev

@konste
Copy link
Contributor

konste commented Jul 12, 2020

I'd like to clarify the logic behind the new attributes - package_file_name and naming_info.
When neither of the two is specified the name of the rule is still used as the name of the package - this is for backwards compatibility.
Now what if only package_file_name is specified? This means instead of the name of the rule we need to take the string specified by package_file_name as the base for the package name. In many cases it is going to be sufficient and people don't need to mess with the new provider at all.
When both package_file_name and naming_info are specified it means: 1. name of the package should be taken from package_file_name and 2. package_file_name and other string typed attributes of the rule should be interpreted as the templates subject to expansion from the dictionary provided by naming_info.

Important points:

  1. package_file_name can be specified without naming_info.
  2. naming_info should affect not only package name, but also other string attributes. package_dir is especially important.

Do you agree?

@aiuto
Copy link
Collaborator Author

aiuto commented Jul 13, 2020 via email

@konste
Copy link
Contributor

konste commented Jul 13, 2020

For tar and zip files, it would only be the file name.

Wait, wait, what about package_dir attribute? Without it being dynamically configurable pkg_tar would still be unusable in our case. I thought you want to get rid of package_dir_file or at least deprecate using it and dynamically configurable package_tar would be a good replacement.

@aiuto
Copy link
Collaborator Author

aiuto commented Jul 13, 2020 via email

@konste
Copy link
Contributor

konste commented Jul 13, 2020

Oh, this is deeper than I was thinking about it. When I discovered package_dir I liked it because it is easy to understand and does exactly what one would expect it to do. I was able to make it do what I need from the first try. When I figured we need it dynamic I dug further and found package_dir_file. It was not an elegant solution, but it worked and unblocked us, so ok.

Now you are talking about matters like package redirection and pkg_filegroup which I simply not aware of.
It would be a pity if in order to make things right we would make them harder to learn and use.
Hope we can keep an easy way for those who need just that and more powerful API for those who need more.

@aiuto
Copy link
Collaborator Author

aiuto commented Jul 13, 2020

I want the fewest number of constructs that work similarly across all the rules. If we want to build a binary that goes in /usr/lib/foo, it would be better to say "prefix everything with /usr/lib/foo" in one place (or at least with one syntax) rather than in every use of that place.

@konste
Copy link
Contributor

konste commented Jul 13, 2020

Unfortunately I don't quite understand your example. package_dir is about internal folder structure of the archive. I understand that there better be one way to specify that info across all packaging rules, but I am not aware of other possible ways besides package_dir.

@nacl
Copy link
Collaborator

nacl commented Aug 10, 2020

cc/ @nacl My question is if package_dir is the right abstraction. Should we have the packaging rules to the package redirection or should pkg_filegroup be the place where that happens?

Sorry for the realllly long delays in responding to this.

In the purest possible sense, pkg_filegroups are intended to define structure, while the packaging rules themselves would be most interested in actually consuming pkg_filegroups and emitting various per-package metadata.

Something like a package_dir attribute is definitely convenient, though: it saves the package authors from potentially writing additional pkg_filegroup rules to define package prefixes. As pkg_filegroup currently stands, we would need something like it (since it's not nestable), but if we happened to have something like what's mocked in #212, packaging rule attributes like package_dir attribute won't be necessary.

The pkg_filegroup design, therefore, should be given some strong scrutiny to make sure we get all the use cases ironed out.

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.

I like the ideas behind this, but I think the templating framework could be used by pkg_filegroups too.

pkg/pkg.bzl Outdated Show resolved Hide resolved
pkg/providers.bzl Outdated Show resolved Hide resolved
aiuto and others added 2 commits August 26, 2020 22:41
* Add attribute strip_prefix to pkg_zip

* Add tests for pkg_zip strip_prefix behavior
@aiuto
Copy link
Collaborator Author

aiuto commented Aug 28, 2020 via email

@aiuto
Copy link
Collaborator Author

aiuto commented Sep 25, 2020

I have something now that I think I want to proceed with.

It demonstrates 3 key new features:

  • to have the file name created by a pkg_ rule be different from the label name
  • to allow variable substitution into that file name, where the variables can dynamically come from the build configuration
  • That downstream rules can, by default, consume the archive produced by a package using the label name even if the actual filename is different.

This PR does not

  • address the package_dir and package_dir_file attributes. Those impact the paths created inside the archive. That is a wholly different set of features. That should be handled in pkg_filegroup.
  • do anything expect pkg_tar. There is no point doing it everywhere before we nail down design.
  • try to dynamically use anything except the target cpu. We can address other platform and toolchain provided values in followup CLs as needed.
  • deal with variables in rpm .spec file expansion. That should be relatively trivial one the infrastructure here is in place.

@aiuto aiuto changed the base branch from master to main September 25, 2020 01:39
@aiuto aiuto changed the title PoC: PackageNamingInfo - provide capability to rename archives based configuration values Provide capability to rename archives based on configuration values Sep 25, 2020
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.

I like where this is going.

I think this needs a little cleanup and clarification on some points before proceeding. Documentation and testing can come later if you'd like.

pkg/package_variables.bzl Outdated Show resolved Hide resolved
pkg/package_variables.bzl Outdated Show resolved Hide resolved
pkg/pkg.bzl Show resolved Hide resolved
pkg/pkg.bzl Outdated Show resolved Hide resolved
pkg/pkg.bzl Show resolved Hide resolved
pkg/pkg.bzl Show resolved Hide resolved
@aiuto aiuto removed the WIP label Sep 28, 2020
@aiuto
Copy link
Collaborator Author

aiuto commented Sep 28, 2020

Thanks for the review. Back to you now.

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.

Changes look good.

@aiuto aiuto merged commit cd2fd76 into bazelbuild:main Sep 30, 2020
@aiuto aiuto deleted the naming branch September 30, 2020 02:32
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.

4 participants