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

bundler: Support prefetching binary dependencies #673

Merged
merged 6 commits into from
Oct 22, 2024

Conversation

a-ovchinnikov
Copy link
Collaborator

@a-ovchinnikov a-ovchinnikov commented Oct 3, 2024

Some dependencies contain architecture-specific components. Their name and location differ from standard Ruby-only dependencies. As a result prior to this commit such dependencies would be skipped during prefetch. This, in turn, failed hermetic builds because a dependency would end up missing.

Resolves: #672

Maintainers will complete the following section

  • Commit messages are descriptive enough
  • Code coverage from testing does not decrease and new code is covered
  • Docs updated (if applicable)
  • Docs links in the code are still valid (if docs were updated)

Note: if the contribution is external (not from an organization member), the CI
pipeline will not run automatically. After verifying that the CI is safe to run:

@a-ovchinnikov
Copy link
Collaborator Author

/retest

@brunoapimentel
Copy link
Contributor

/retest

Copy link
Collaborator

@slimreaper35 slimreaper35 left a comment

Choose a reason for hiding this comment

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

It would be fine to keep e2e tests as separate commits. You forgot to generate test data.

cachi2/core/package_managers/bundler/main.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/bundler/parser.py Outdated Show resolved Hide resolved
tests/integration/test_bundler.py Show resolved Hide resolved
@slimreaper35
Copy link
Collaborator

slimreaper35 commented Oct 10, 2024

I am not sure if overall that's what we want. When I was testing it locally, it looked like our parser script always adds platform = "ruby" if the source gem exists. So if users set allow binary to true, we download source gems anyway.

When I had (sorbet)[https://rubygems.org/gems/sorbet-static/versions] in Gemfile.lock. JSON output had 3 gems for 3 different platforms. Then, we would not download all pre-compiled gems available as we do in pip.

Everything kind of depends on the parser.

@a-ovchinnikov
Copy link
Collaborator Author

I am not sure if overall that's what we want.

My understanding is that we want to download whatever is specified in a Gemfile.lock. Technically we do not even need to ask a user if they want precompiled binaries or not, but for consistency with pip we do this. I think it is also good to be explicit about binary blobs and make user acknowledge that this is indeed what they want: there is always a chance that this was never noticed before and made its way into a stricter environment by accident.

cachi2/core/package_managers/bundler/parser.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/bundler/parser.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/bundler/parser.py Outdated Show resolved Hide resolved
tests/unit/package_managers/bundler/test_parser.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/bundler/parser.py Outdated Show resolved Hide resolved
cachi2/core/package_managers/bundler/parser.py Outdated Show resolved Hide resolved
cachi2/core/models/input.py Show resolved Hide resolved
cachi2/core/models/input.py Show resolved Hide resolved
Copy link
Member

@eskultety eskultety left a comment

Choose a reason for hiding this comment

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

Mostly minor comments on top of @slimreaper35's . The default bundler version downgrade behaviour raised a red flag for me and so I believe we need to set BUNDLE_VERSION.

cachi2/core/models/input.py Show resolved Hide resolved
cachi2/core/package_managers/bundler/main.py Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
This class represents binary dependencies which some gems have.

Signed-off-by: Alexey Ovchinnikov <[email protected]>
Copy link
Collaborator

@slimreaper35 slimreaper35 left a comment

Choose a reason for hiding this comment

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

I miss generated SBOM from e2e tests.

@eskultety eskultety changed the title bundler: Fix for prefetching of dependencies bundler: Support prefetching binary dependencies Oct 18, 2024
Some dependencies contain architecture-specific components.  Their name
and location differ from standard Ruby-only dependencies. As a result
prior to this commit such dependencies would be skipped during prefetch.
This, in turn, failed hermetic builds because a dependency would end up
missing.

Resolves: containerbuildsystem#672

Signed-off-by: Alexey Ovchinnikov <[email protected]>
@slimreaper35
Copy link
Collaborator

One more thing, that I probably mentioned somewhere, if we want to add platform qualifier in the PURLs:
https://github.com/package-url/purl-spec/blob/master/PURL-TYPES.rst#gem

Some gems have hard dependencies on binary-only gems
(at the moment of writing sorbet was one such gem).
This commit introduces a way to allow downloading binary
gems by adding an allow_binary flag along the lines
of pip's allow_binary flag.

Signed-off-by: Alexey Ovchinnikov <[email protected]>
When there is a version mismatch between a lockfile and bundler
used to build a package, bundler might try to install a version
specified in the lockfile. This will result in a build failure
in a hermetic environment. This change forces bundler to
always use whichever version is present in the system by overriding
BUNDLE_VERSION variable and setting it to 'system'. See
  https://bundler.io/v2.5/man/bundle-config.1.html#LIST-OF-AVAILABLE-KEYS
for details.

Signed-off-by: Alexey Ovchinnikov <[email protected]>
This commit adds e2e to bundler. The tests verify that
gems pre-fetched with cachi2 could be built in isolation.

Signed-off-by: Alexey Ovchinnikov <[email protected]>
@eskultety
Copy link
Member

One more thing, that I probably mentioned somewhere, if we want to add platform qualifier in the PURLs: https://github.com/package-url/purl-spec/blob/master/PURL-TYPES.rst#gem

Good point.

@a-ovchinnikov
Copy link
Collaborator Author

One more thing, that I probably mentioned somewhere, if we want to add platform qualifier in the PURLs: https://github.com/package-url/purl-spec/blob/master/PURL-TYPES.rst#gem

Good point.

If I read the link correctly this is a different platform: not a platform a gem is compiled to, but a Ruby implementation. The given example explicitly mentions JRuby.

@slimreaper35
Copy link
Collaborator

slimreaper35 commented Oct 21, 2024

If I read the link correctly this is a different platform: not a platform a gem is compiled to, but a Ruby implementation. The given example explicitly mentions JRuby.

Okay. So we can probably leave it. We are not handling this for pip either.
And how about the SBOM from e2e tests ?

@a-ovchinnikov
Copy link
Collaborator Author

And how about the SBOM from e2e tests ?

Do we need it? The logic of SBOM generation is pretty static and well-tested with UTs (97+% coverage is no joke). The tests check that bundler can build a package after it was prepared by cachi2 and that there are no failures there, they do catch quite a lot of interesting behaviours as we have learned. I think adding more data is excessive and will not bring any more confidence.

@slimreaper35
Copy link
Collaborator

We have it for all e2e tests for all package managers, so at least for consistency, I would add it. We can discuss it when we put all integration tests into a single repo. That should be a good starting point for improving the integration tests overall.

A snapshot of e2e data for e2e tests.

Signed-off-by: Alexey Ovchinnikov <[email protected]>
@a-ovchinnikov
Copy link
Collaborator Author

We have it for all e2e tests for all package managers

Done!

@a-ovchinnikov a-ovchinnikov added this pull request to the merge queue Oct 22, 2024
Merged via the queue into containerbuildsystem:main with commit ad353e5 Oct 22, 2024
16 checks passed
@a-ovchinnikov a-ovchinnikov deleted the issue672 branch October 22, 2024 13:56
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.

Bundler PM does not prefetch platfrom-specific gems
4 participants