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

Inconsistent cargo bazel lockfile between MacOS and Linux #2212

Closed
jvliwanag opened this issue Oct 25, 2023 · 5 comments · Fixed by #2877
Closed

Inconsistent cargo bazel lockfile between MacOS and Linux #2212

jvliwanag opened this issue Oct 25, 2023 · 5 comments · Fixed by #2877

Comments

@jvliwanag
Copy link

Given a rust project with the dependency: pbjson_types, the generated cargo bazel lockfile differs when generated on Mac and Linux.

See sample project here. The diff switching from the mac generated one to the linux one is on this commit.

Mac build is from an M2 Sonoma. Linux build is from a docker ubuntu arm64 image.

@gferon
Copy link
Contributor

gferon commented Oct 25, 2023

I encountered the exact same issue in the past few weeks. It is related to a (current) hard limitation of cargo metadata which does not expose features per target platform. Because of that, rules_rust relies on cargo tree to determine which features are enabled for each platform.

The problem with this approach is that cargo tree will always compute build dependencies for the host platform, which in your case can either be macOS or Linux.

One quick & dirty solution could be to add a --host-triple to cargo tree to be able to list build-dependencies for each platform Bazel is going to build on and for. I started a discussion with the cargo maintainers which pointed me towards cargo metadata.

It looks like we could improve cargo_universe a lot if cargo metadata would be able to use resolver = 2 but this sounds complicated, see: https://rust-lang.github.io/rfcs/2957-cargo-features2.html#cargo-metadata

This whole story was already discovered by @illicitonion a while ago in #1662 - could you maybe give your two cents to help move in one direction or another?

Some extra relevant links:

@jvliwanag
Copy link
Author

Thanks for looking into this. As this seems blocked though by changes we’d need on cargo, what would be viable options to workaround this issue for the meantime?

@illicitonion
Copy link
Collaborator

One quick & dirty solution could be to add a --host-triple to cargo tree to be able to list build-dependencies for each platform Bazel is going to build on and for.

This sounds like the right approach (until cargo metadata itself gets fixed) - we already have a list of the platforms we care about, and we're already serializing this as a SelectList, so running those cargo-trees and adding each resolved feature for each platform sounds reasonable.

@jvliwanag
Copy link
Author

I’ve gotten around the issue for now by running cargo sync on the different platforms and adding them manually via annotations.

As such, looks like whatever would automate this would work for my case.

@gferon
Copy link
Contributor

gferon commented Nov 2, 2023

One quick & dirty solution could be to add a --host-triple to cargo tree to be able to list build-dependencies for each platform Bazel is going to build on and for.

This sounds like the right approach (until cargo metadata itself gets fixed) - we already have a list of the platforms we care about, and we're already serializing this as a SelectList, so running those cargo-trees and adding each resolved feature for each platform sounds reasonable.

I touched base with some cargo maintainers and didn't get very far with either solution. I'll keep thinking about how to proceed further.

ParkMyCar added a commit to MaterializeInc/materialize that referenced this issue Jul 22, 2024
This PR makes a number of small changes to our Bazel setup:

1. Updates `cargo-gazelle` so it generates the correct amount of
newlines
2. Changes the `test_output` setting in `.bazelrc` from `streamed` to
`all`, `streamed` looked nice but forced the execution of tests to be
serial.
3. Change the ci-builder image to read the Bazel version from the
`.bazelversion` file
4. Update `.bazelrc` to add some settings for remote caching
5. Upgrade to rules_rust `v0.48` (previously we were on `v0.44`)
6. Delete `Cargo.Bazel.lock`. Ideally we would include the lock file,
but unfortunately the lockfile generation can be inconsistent between
macOS and Linux, see
bazelbuild/rules_rust#2212. Our builds are
still based off of the `Cargo.lock` file, the only difference is it
takes an extra couple seconds to re-gen BUILD files for third party
crates. A fix for this is either switching to `crates_vendored` or using
Bazel Modules. We can do either in a follow up.
7. Add more settings to `.bazelrc`, taking suggestions from what
[here](https://blog.aspect.build/bazelrc-flags)
8. Update the Rocksdb build so `librocksdb-sys` doesn't build `snappy`
on its own and instead uses the one built by Bazel

### Motivation

This is the final step before committing our generated `BUILD.bazel`
files.

### Tips for reviewer

It's easier if you review commit by commit.

### Checklist

- [ ] This PR has adequate test coverage / QA involvement has been duly
considered. ([trigger-ci for additional test/nightly
runs](https://trigger-ci.dev.materialize.com/))
- [ ] This PR has an associated up-to-date [design
doc](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/README.md),
is a design doc
([template](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/00000000_template.md)),
or is sufficiently small to not require a design.
  <!-- Reference the design in the description. -->
- [ ] If this PR evolves [an existing `$T ⇔ Proto$T`
mapping](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/command-and-response-binary-encoding.md)
(possibly in a backwards-incompatible way), then it is tagged with a
`T-proto` label.
- [ ] If this PR will require changes to cloud orchestration or tests,
there is a companion cloud PR to account for those changes that is
tagged with the release-blocker label
([example](MaterializeInc/cloud#5021)).
<!-- Ask in #team-cloud on Slack if you need help preparing the cloud
PR. -->
- [x] This PR includes the following [user-facing behavior
changes](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/guide-changes.md#what-changes-require-a-release-note):
  - N/a
github-merge-queue bot pushed a commit that referenced this issue Sep 18, 2024
This change expands the use of `cargo tree` to resolve features for
different combinations of `host -> target` platform triples where before
`cargo-bazel` was only capable of resolving host dependencies for the
current host platform and not a foreign platform.

A lengthy description of how this was done can be found at
`crate_universe/src/metadata/cargo_tree_resolver.rs -
TreeResolver::create_rustc_wrapper`.

closes #2212

---------

Co-authored-by: Daniel Wagner-Hall <dawagner@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants