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 pkg_sub_rpm rule for RPM subpackages #824

Merged
merged 18 commits into from
Mar 15, 2024
Merged

Conversation

kellyma2
Copy link
Contributor

This change introduces a pkg_sub_rpm rule that can be used to generate the sub RPMs associated with a given parent RPM. Currently this would be possible to cobble together using multiple pkg_rpm rules, but doing it "correctly" as sub RPMs has a few advantages:

  • faster execution due to single rpmbuild invocation
  • configuration (eg: version) is shared between all RPMs in the group
  • possible to properly generate debuginfo RPMs using rpmbuild

This implementation explicitly only works with the non-specfile based invocations of pkg_rpm so as to avoid having to scrape the specfile in order to determine which subpackages we're generating.

Internally, the rule functions by generating a PackageSubRPMInfo provider that can then be consumed by our top-level pkg_rpm rule to perform the actual RPM generation.

Before we can enable support for sub RPM building as part of a single
`pkg_rpm()` rule we must add the underlying support to make_pkg.py
which is the underlying driver for `pkg_rpm()`.

This covers three pieces:

 * specifying `buildsubdir` rpm variable
 * capturing multiple RPM files as outputs
 * injecting sub RPM definitions into specfile
The various processing functions pass around a bunch of collections
everywhere which is a bit fragile.  This collects them together into a
struct to make it a bit less messy.
_process_dep() handles processing an individual dep and is currently
called from the processing loop.  We'll need to re-use this logic for
processing individual sub RPMs as well so we want it in a helper.
Currently we only generate one RPM file, but once we generate sub RPM
files we'll need to be able to capture those outputs as well.  This
prepares us for that step.
We'll need to add additional arguments to make_rpm for sub RPM
building.  It's easier to capture this in our context object than to
try to shuttle these bits around.
If we don't pass the correct RPM name to `make_rpm.py` than we won't be
able to correctly determine the subrpm names.  Currently, the name is
only used by `make_rpm` to generate some progress output, so this
shouldn't break anything internally.
This introduces a `pkg_sub_rpm` rule that allows us to generate and
capture RPM subpackages and generate them as part of a single RPM
invocation in lieu of cobbling this together from multiple RPM rules.
This has a few benefits:

- faster execution due to single rpmbuild invocation
- sharing configuration between RPMs in the same fashion as vanilla
  RPM building from a specfile
- will enable the proper construction of debuginfo RPMs in a later PR

The current implementation *only* works with non-specfile based rules
and currently allows for a subset of the general RPM configuration.

Internally, the process is for `pkg_sub_rpm` to generate a
PackageSubRPMInfo provider that will subsequently get consumed by the
`pkg_rpm` rule that generates the actual RPMs.  We re-use the internal
dependency processing logic that's used by the top-level RPM to
generate the content related to the sub-RPM.
This change updates `rpm.bzl` in two ways to account for sub RPMs:

- it adds an entrypoint for `pkg_sub_rpm`

- it adds a check in `pkg_rpm` to assert incompatibility with
  `spec_file`
This provides a basic example using the `pkg_sub_rpm` rule to generate
multiple RPMs.
@cgrindel
Copy link
Collaborator

This implementation explicitly only works with the non-specfile based invocations of pkg_rpm so as to avoid having to scrape the specfile in order to determine which subpackages we're generating.

I did not notice this being called out in any doc. Is it worth mentioning it somewhere so that folks do not go down a rabbit hole?

@kellyma2
Copy link
Contributor Author

@aiuto do you prefer for me to add the doc updates in this PR or should I leave that to be updated separately?

The initial docstring for pkg_sub_rpm is not great, so this change
fixes that while adding this to the list of rules to have
documentation generated for them.
@kellyma2 kellyma2 requested a review from jylinv0 as a code owner February 29, 2024 18:20
This clarifies the `subrpms` attribute usage as well as indicating the
incompatibility with `spec_file` mode.
@kellyma2
Copy link
Contributor Author

This implementation explicitly only works with the non-specfile based invocations of pkg_rpm so as to avoid having to scrape the specfile in order to determine which subpackages we're generating.

I did not notice this being called out in any doc. Is it worth mentioning it somewhere so that folks do not go down a rabbit hole?

The last commit in the list here adds that clarification to the pkg_rpm rule docstring.

kellyma2 added 4 commits March 5, 2024 15:34
When adding the check to verify if we can use subrpms, this pass
through wasn't added.
This introduces a basic test with a single sub RPM and main RPM each
containing a single source file.
The `rpm` version on CentOS 7 doesn't work exactly the same way as
newer versions of rpm and requires a `-p` parameter to inspect
non-installed RPMs.  CentOS 7 also inserts a `Relocations` field that
we didn't see on other platforms so we'll filter that out to make our
test a bit more portable.
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.

Sorry for the slow reply. I started a new role last week and have been swamped.

pkg/providers.bzl Outdated Show resolved Hide resolved
@aiuto
Copy link
Collaborator

aiuto commented Mar 6, 2024

This implementation explicitly only works with the non-specfile based invocations of pkg_rpm so as to avoid having to scrape the specfile in order to determine which subpackages we're generating.

I did not notice this being called out in any doc. Is it worth mentioning it somewhere so that folks do not go down a rabbit hole?

It sounds like the implementation does not do it, but there is no reason why it could not in some way. Would that mean taking the provided specfile and splicing in new sections for each sub rpm?

This is private to the RPM rules so should probably live there.
@kellyma2
Copy link
Contributor Author

kellyma2 commented Mar 6, 2024

Sorry for the slow reply. I started a new role last week and have been swamped.

No worries :)

@kellyma2
Copy link
Contributor Author

kellyma2 commented Mar 6, 2024

This implementation explicitly only works with the non-specfile based invocations of pkg_rpm so as to avoid having to scrape the specfile in order to determine which subpackages we're generating.

I did not notice this being called out in any doc. Is it worth mentioning it somewhere so that folks do not go down a rabbit hole?

It sounds like the implementation does not do it, but there is no reason why it could not in some way. Would that mean taking the provided specfile and splicing in new sections for each sub rpm?

Anything is possible in theory, but I expect it'd be a bit messy. I hadn't looked in depth at the legacy/specfile mode as I was assuming this is not the encouraged way to move forward.

That noted, I did add this caveat to the docstrings for the rules to ensure that this is more clear and there's a guard in there to prevent the combination of subrpms and specfile mode.

@kellyma2 kellyma2 requested a review from aiuto March 6, 2024 05:25
@kellyma2
Copy link
Contributor Author

@aiuto Any updates?

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.

Approval on the grounds that I'm expecting you to fix any bugs people find with this. :-)

@aiuto aiuto merged commit 2aa2b8e into bazelbuild:main Mar 15, 2024
2 checks passed
@kellyma2
Copy link
Contributor Author

Approval on the grounds that I'm expecting you to fix any bugs people find with this. :-)

Sounds fun :)

@aiuto
Copy link
Collaborator

aiuto commented Mar 15, 2024 via email

@kellyma2 kellyma2 deleted the pkg_sub_rpm branch March 15, 2024 20:54
@kellyma2
Copy link
Contributor Author

It turns out this breaks on the version of rpmbuild I have. Sigh.

From testing my debuginfo changes, it seems like this is likely more dependant on the underlying system then it is on the rpmbuild version. Specifically, I got very different results from CentOS and Fedora because of how they've defined the RPM macros. Definitely needs more testing. :P

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