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

Support providing integrity for gem downloads #82

Merged
merged 2 commits into from
Feb 28, 2024

Conversation

sushain97
Copy link
Collaborator

@sushain97 sushain97 commented Feb 25, 2024

Our remote downloader service categorically rejects all requests to download a blob without a checksum. Until #63 is complete, users will need to be able to specify gem checksums manually. This PR makes that possible by adding an gem_checksums dict to the rb_bundle_fetch rule and Bazel will direct users to use it:

~/bazel-contrib.rules_ruby/examples/gem ❯❯❯ bazel build ...
Starting local Bazel server and connecting to it...
DEBUG: Rule 'rules_ruby~override~ruby~bundle' indicated that a canonical reproducible form can be obtained by modifying arguments integrity = {"ast-2.4.2": "sha256-HigCMuajN1TN5UK8XvhVILdNsqrHPsFKzvRTeERHzBI=", "concurrent-ruby-1.2.2": "sha256-OHkRm4t147YmFqzCVsZKE00LCnqaP8ulojMCW83iLE8=", "debug-1.6.3": "sha256-M18Qmr45weNGXfLS8X9RCOKTQHBELnPAoU0J3lQBI0E=", "diff-lcs-1.5.0": "sha256-Sbk0AByMau2ze6GdrsXGNNonsxino8ZUrpeda6GSm2c=", "i18n-1.14.1": "sha256-nQNpiQNUfAYJKOcKm8i2uH/aZ0RTzakY/Hq4AjWuSmE=", "io-console-0.5.11": "sha256-fiQYN2/Rha1m567ixYwgfpvg8hl6qJvEyJkxmVzuM2U=", "io-console-0.5.11-java": "sha256-WHFcgGylfEKyKQy0vh+3DypT1YD2+f5GC5r4ju2EDbg=", "irb-1.5.0": "sha256-7Y4Vq5cDSn3hxfwmND8Og8Q9hcdZ2XJF8Ztr17xiuvY=", "jar-dependencies-0.4.1": "sha256-st8vHsv/FTNM4g6n/dW42BYfqrZ3Yf9yx2R9co5A04c=", "json-2.6.3": "sha256-hqrqFq3zRqKyJ0PYj43M7rEDiEOYmrk82kS1F2yEVFk=", "json-2.6.3-java": "sha256-6oxHQnosh2EhuaCrUwQ8o5ABOnY3QzDqvZI72BkU5WM=", "language_server-protocol-3.17.0.3": "sha256-PVxYwC9Eog2XKVep/r44bX50aKs5AM5r0rVj3ZEMaz8=", "parallel-1.23.0": "sha256-JxVHE61u8y+j3Ld4inIdbAe8p35yRDtMYIChQUUojEk=", "parser-3.2.2.3": "sha256-EGhfNYqzb/6iJS3ElS5bj606KXqBUqhfWa3JgnR7kes=", "psych-5.1.1.1": "sha256-RLDRgjYprIFfH0cK9kLccmFInWf+tiKj9Vc6qfXMX3I=", "psych-5.1.1.1-java": "sha256-DmdVLGQGmDWmlmlt6dcMmoN1HF2OKnaHxtRY6opZMcc=", "racc-1.7.1": "sha256-r2QSSDb908AOgwcD1/hz6l3qvekj83AGo59aXg2hY4c=", "racc-1.7.1-java": "sha256-6qXNEKzjalxaE55pmHWkX6Hf19XfhDL/1iQ5YsayTvA=", "rainbow-3.1.1": "sha256-A5SRqjqJ9C76HW3sL8TmLt6W62rNleUvGtWBGCt5vGo=", "rake-13.1.0": "sha256-vmo+Gqf2bmxl+ldVUjTrdc5M9K2gd2WESSByBUdBmcY=", "regexp_parser-2.8.1": "sha256-g/Y+K/rj2zj5iMZvEUSFFA/xeRMh/YJ0gLx1qkLKy4w=", "reline-0.3.1": "sha256-sQHZNge/dWRlfwgvaKv6Ga6TnRSnCe/4m+BI6uLX9Mc=", "rexml-3.2.5": "sha256-ozw7+V/aeYPsfwUFTzqYWvQdvCWgM5hDvSR56TyrsSM=", "rspec-3.11.0": "sha256-iQfzLlwwlXJOVMFD6c/E3zcHm+NJND2rkCm+zbJZvqU=", "rspec-core-3.11.0": "sha256-RjF4UDlv6kfmeT3Vp2BsCBaqOPUUn0zV3jCElbibEIU=", "rspec-expectations-3.11.1": "sha256-bA28VgpK6o4Fjj3CYzZXcl2QAdF7oB6xWCEio8z8K1A=", "rspec-mocks-3.11.1": "sha256-VTfcBpr6vOpcvBmaFDKidyujpGXzIz9A0EaV2rp8ah8=", "rspec-support-3.11.1": "sha256-+F8najfglre/bzCKlsWboKU5iQfwSULO3mbKvFmMAbA=", "rubocop-1.55.0": "sha256-cd79tEyEC1gNtUGQDgIZTYerfm81GSIdcR8vJSgniZ0=", "rubocop-ast-1.29.0": "sha256-0doqsnmgdLrvyBdYrEMMV2io2ox0ON1OWBnOWYTQC6E=", "ruby-progressbar-1.13.0": "sha256-gPycR6m2QNaDTg3Hs8lMnfN/CMsHK3dh5KceIs/ymzM=", "stringio-3.0.9": "sha256-5zmFWOPFQJRHsUe9WwMaN1YW9EOICus1fE5N8Moj7uU=", "unicode-display_width-2.4.2": "sha256-ahAgXRoZynkMTlMGS6k/CdnrI0v2vRNdnetgAcIUKL4=", "bundler-2.2.19": "sha256-MlgwG4VJX0J5SdjYJZJtgXOSzKZT8FR56qeEX3HAiqA="}
DEBUG: Repository rules_ruby~override~ruby~bundle instantiated at:
  <builtin>: in <toplevel>
Repository rule rb_bundle_fetch defined at:
  /home/sushain/.cache/bazel/_bazel_sushain/7810a26d8d100142d3052996d63d34e5/external/rules_ruby~override/ruby/private/bundle_fetch.bzl:213:34: in <toplevel>

Even with #63 complete, I think it's valuable to provide users that aren't on the latest version of Bundler a way to provide checksums. As part of that issue, we can make the returned gem_checksums value only require whatever checksums are not present in the Gemfile.lock.

Users that don't want a big dict in their WORKSPACE can easily locate it elsewhere in a checksums.bzl and load it up to use here.

Copy link
Member

@p0deje p0deje left a comment

Choose a reason for hiding this comment

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

Users that don't want a big dict in their WORKSPACE can easily locate it elsewhere in a checksums.bzl and load it up to use here.

Unfortunately this is not possible with Bzlmod because load() is not allowed in MODULE.bazel. The way around this would be to support passing a file input to the integrity, which is later read by a repository rule:

rb_bundle_fetch(
  integrity = "//:checksums.bzl"
)

WIth that said, what do you think if we change the PR to:

  1. Rename integrity to checksums.
  2. Require it to be put into a file rather than a dict.
  3. Make syntax similar to Gemfile.lock to avoid writing 2 parsers:
ast (2.4.2) sha256=1e280232e6a33754cde542bc5ef85520b74db2aac73ec14acef453784447cc12

@sushain97
Copy link
Collaborator Author

sushain97 commented Feb 25, 2024

@p0deje that's a shame re. Bzlmod.

I think there are a couple questions I have about the file-based approach:

  1. Would it still be possible to provide users a way to copy-paste a snippet that would make their build deterministic? By returning the integrity attribute all filled in, that makes it easy today. For a file, we'd have to add some kind of runnable target and then prompt users to run it via a print...? Feels pretty unfortunate.
  2. Should we just not support a separate file at all? If a user is on an older Bundler version, they can write their own tiny rule to cat a Gemfile.lock and a GemChecksums file? If you'd prefer minimizing the surface area of these rules, I'm OK with that. But, I don't know how large the population of folks that use older versions of Bundler/Ruby is and therefore how annoying the 'write it yourself' approach would be.
  3. What about the checksum for bundler itself. Should we make that another attribute? It won't be in the Gemfile.lock at all.

@p0deje
Copy link
Member

p0deje commented Feb 25, 2024

Would it still be possible to provide users a way to copy-paste a snippet that would make their build deterministic? By returning the integrity attribute all filled in, that makes it easy today. For a file, we'd have to add some kind of runnable target and then prompt users to run it via a print...? Feels pretty unfortunate.

I feel like the checksums.bzl would rather be discouraged over using built-in Gemfile.lock checksums, so I'd rather not complicate the implementation with extra runnable targets.

Should we just not support a separate file at all? If a user is on an older Bundler version, they can write their own tiny rule to cat a Gemfile.lock and a GemChecksums file? If you'd prefer minimizing the surface area of these rules, I'm OK with that. But, I don't know how large the population of folks that use older versions of Bundler/Ruby is and therefore how annoying the 'write it yourself' approach would be.

That's an option too. As I mentioned before, I feel like 99% should just upgrade to latest Bundler and use Gemfile.lock as a source of checksums. Frankly, I'd suggest printing this message if CHECKSUMS is missing in Gemfile.lock.

What about the checksum for bundler itself. Should we make that another attribute? It won't be in the Gemfile.lock at all.

I believe that the ruleset itself should have the checksums for all versions of bundler, while still allowing to pass it as an attribute to rb_bundle_fetch(...).

@sushain97
Copy link
Collaborator Author

That's an option too. As I mentioned before, I feel like 99% should just upgrade to latest Bundler and use Gemfile.lock as a source of checksums. Frankly, I'd suggest printing this message if CHECKSUMS is missing in Gemfile.lock.

That sounds reasonable. I can just do that.

Upgrading bundler is a whole production at my shop so I'll probably just create a synthetic Gemfile.lock for now. I'll switch this PR into resolving #63 then.

I believe that the ruleset itself should have the checksums for all versions of bundler, while still allowing to pass it as an attribute to rb_bundle_fetch(...).

I'm happy with that. I'll send a separate PR for that after #81 is complete since that change will conflict with #81.

@p0deje
Copy link
Member

p0deje commented Feb 25, 2024

Upgrading bundler is a whole production at my shop so I'll probably just create a synthetic Gemfile.lock for now.

Hmm, I tried upgrading to 2.5.3 but still don't get CHECKSUMS in Gemfile.lock. I wonder if there is anything to be done in Bundler to turn this feature on.

@sushain97 sushain97 mentioned this pull request Feb 27, 2024
@sushain97
Copy link
Collaborator Author

Upgrading bundler is a whole production at my shop so I'll probably just create a synthetic Gemfile.lock for now.

Hmm, I tried upgrading to 2.5.3 but still don't get CHECKSUMS in Gemfile.lock. I wonder if there is anything to be done in Bundler to turn this feature on.

Does it require running bundle lock manually?

@p0deje
Copy link
Member

p0deje commented Feb 27, 2024 via email

@p0deje
Copy link
Member

p0deje commented Feb 28, 2024

Ok, given the fact that I can't make Bundler 2.5 generate a CHECKSUMS section, let's go with this PR. Can you resolve the conflicts and update examples/jekyll to also include integrity?

@sushain97
Copy link
Collaborator Author

Ok, given the fact that I can't make Bundler 2.5 generate a CHECKSUMS section, let's go with this PR. Can you resolve the conflicts and update examples/jekyll to also include integrity?

Yep!

@sushain97
Copy link
Collaborator Author

@p0deje OK, done! I renamed integrity to gem_checksums to match bundler_checksums as well as the eventual CHECKSUMS section.

@p0deje p0deje merged commit 41e0b2d into bazel-contrib:main Feb 28, 2024
43 checks passed
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.

2 participants