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

crate.from_spec module extension is not reproducible #3072

Open
tpudlik opened this issue Dec 10, 2024 · 7 comments
Open

crate.from_spec module extension is not reproducible #3072

tpudlik opened this issue Dec 10, 2024 · 7 comments

Comments

@tpudlik
Copy link
Contributor

tpudlik commented Dec 10, 2024

Protobuf uses the crate.from_spec module extension. Unfortunately, this extension is not reproducible. What this means in practice is that any project that uses protobuf via bzlmod and wishes to check in its lockfile must regenerate that lockfile on all OS/arch combinations it supports.

This is very painful, especially because there is only one MODULE.bazel.lock file (rather than one per OS/arch combination, like Python requirements files), so I believe I can't simply run lockfile-updating CI jobs in parallel and then apply the git patches they generate. (I expect the patches will conflict!) Instead, I'll need to run them one at a time, and apply patches in between.

I see a few possible solutions here:

  1. Have protobuf check in their Cargo lockfile and not use the crate.from_spec extension.
  2. Fix crate.from_spec by specifying -minimal-version, as suggested in the comment @dzbarsky left in Mark the bzlmod extension reproducible as appropriate #2575.
  3. Ensure MODULE.bazel.lock file updates from different OS/arch combinations can always be patched in any order (thus ensuring the patches can be generated in parallel by CI). This would be a FR for bzlmod.

I'm not sure how options 1 and 2 compare in terms of difficulty, or how feasible option 3 is. Are there any better ways out of this situation?

@fmeum @Wyverald

Originally ran into this here: https://pwbug.dev/354274498#comment41

@dzbarsky
Copy link
Contributor

While it would be nice to make the extension reproducible, I think the most pragmatic change is for protobuf to not force these deps on all users. I don't see how anyone should need to depend on rust googletest to use protos (much less in other languages). See also protocolbuffers/protobuf#18825 (comment) and the rest of that PR's discussion

@fmeum
Copy link
Contributor

fmeum commented Dec 10, 2024

Ensure MODULE.bazel.lock file updates from different OS/arch combinations can always be patched in any order (thus ensuring the patches can be generated in parallel by CI). This would be a FR for bzlmod.

This should already be the case if the module extension declares itself as os/arch dependent. Does cargo.from_spec declare this on its module_extension call? What behavior are you observing?

Generally speaking, I fully agree with @dzbarsky here. protobuf needs to do a full survey of their dependencies and try to get rid of everything that isn't strictly needed. Ideally, there would be a pure rules module that has almost no language-specific deps. They said on one issue that they would look into this in H1 2025, so that will take time.

@tpudlik
Copy link
Contributor Author

tpudlik commented Dec 10, 2024

Ah, thank you for the link to that protobuf PR---I didn't realize that the crate.from_spec usage was removed in protobuf's 29.0 release! So it looks like my pain can be solved simply by upgrading from 28.2.

I do agree that a very widely used library like protobuf should strive to have few and easy-to-manage dependencies, which crate.from_spec certainly is not!

I'll leave the issue open because ultimately making crate.from_spec reproducible remains desirable.

@tpudlik
Copy link
Contributor Author

tpudlik commented Dec 10, 2024

Ensure MODULE.bazel.lock file updates from different OS/arch combinations can always be patched in any order (thus ensuring the patches can be generated in parallel by CI). This would be a FR for bzlmod.

This should already be the case if the module extension declares itself as os/arch dependent. Does cargo.from_spec declare this on its module_extension call? What behavior are you observing?

Following up on that specifically: I upgraded the protobuf version I depend on, and then ran bazelisk mod deps --lockfile_mode=update separately on Linux (generating this lockfile diff) and on Windows (generating this lockfile diff). I then tried to git apply the MacOS-generated diff on top of the Linux-generated diff---but the patch did not apply.

@fmeum
Copy link
Contributor

fmeum commented Dec 13, 2024

The patches require a corp sign-in. Have you followed https://bazel.build/external/lockfile#automatic-resolution?

Merging changes of the form ABC -> ABDC and ABC -> ABEC with regular merge logic isn't possible since it's not clear how D and E should be sorted relative to each other. The merge driver knows that sorting lexicographically is fine.

@tpudlik
Copy link
Contributor Author

tpudlik commented Dec 13, 2024

Ah, sorry about the patch links, fixed! I didn't know about the merge driver. I'll give it a try. It's too bad it requires all developers to install an external dependency by hand.

@fmeum
Copy link
Contributor

fmeum commented Dec 13, 2024

We could potentially make the format more mergeable by always including (empty) sections for all supported OS/arch combinations as this should solve the ordering problem. Could you try that out on your two patches?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants