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 cargo_bazel_bootstrap for bzlmod #2021

Merged
merged 4 commits into from
Oct 25, 2023

Conversation

matts1
Copy link
Contributor

@matts1 matts1 commented Jun 19, 2023

I've pulled out some of the code from #1910 into a seperate PR, to simplify the review process.

Copy link

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

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

Cool! I'll test it against an internal codebase but this looks like it will just work.

Not sure how to handle documentation in this case. AFAIK stardoc doesn't support doc generation for extensions yet, but a short description or just links to the WORKSPACE equivalents in the extension doc fields would probably be helpful.

@matts1 matts1 force-pushed the cargo_bazel_bootstrap branch 2 times, most recently from 17074ce to f4804b4 Compare June 25, 2023 23:17
@matts1
Copy link
Contributor Author

matts1 commented Jun 25, 2023

Added a doc tag to the module_extension call

@matts1
Copy link
Contributor Author

matts1 commented Jun 25, 2023

FYI, the command to test it should be:
bazel build @cargo_bazel_bootstrap//:binary --enable_bzlmod

@aaronmondal
Copy link

aaronmondal commented Jun 26, 2023

Just tested this from the bzlmod example with the following MODULE.bazel:

module(
    name = "hello_world",
    version = "1.0",
)

bazel_dep(name = "rules_rust", version = "0.9.0")
local_path_override(
    module_name = "rules_rust",
    path = "../../..",
)

rust = use_extension("@rules_rust//rust:extensions.bzl", "rust")
rust.toolchain(edition = "2021")
use_repo(
    rust,
    "rust_toolchains",
    "rust_host_tools",
)

cargo_bazel_bootstrap = use_extension(
    "@rules_rust//crate_universe/private/module_extensions:cargo_bazel_bootstrap.bzl",
    "cargo_bazel_bootstrap",
)
use_repo(cargo_bazel_bootstrap, "cargo_bazel_bootstrap")

register_toolchains("@rust_toolchains//:all")
bazelisk run --enable_bzlmod @cargo_bazel_bootstrap//:binary -- --help

and seems to work fine :) Thanks a lot for this it really simplifies using the rust dependency autogeneration ❤️

BTW @matts1 did you ever encounter issues with the "rust_linux_x86_64__x86_64-unknown-linux-gnu__stable_tools" repo or similar when working on crate_universe-related things? In my experiments I had to add the _tools repo manually in the extension which hardly seems like intended behavior. Maybe that points to issues that, when solved, could simplify code-reuse between the bzlmod and the WORKSPACE crate_universe functionality?

@UebelAndre UebelAndre requested a review from scentini June 26, 2023 13:16
@matts1
Copy link
Contributor Author

matts1 commented Jun 26, 2023

This PR shouldn't touch the "rust_linux_x86_64__x86_64-unknown-linux-gnu__stable_tools" repo - instead, it should generate another repo rust_host_tools with the exact same contents. The repo you mentioned should only be used when attempting to compile a rust_* target, not when using cargo_bazel_bootstrap.

@opicaud
Copy link

opicaud commented Jul 19, 2023

Hello guys,

Very big thanks for this change, i was trying to do it for few days ago already and i was dealing with the famous issue "rust_linux_x86_64__x86_64-unknown-linux-gnu__stable_tools"..

So I think we can go further :)
Basically provide an extension (on client side) to be able to load the bzlmod equivalent of

load("@rules_rust//crate_universe:defs.bzl", "crates_repository")
crates_repository(name = "crate_index", " manifests = ["//:Cargo.toml"], ...)

load("@crate_index//:defs.bzl", "crate_repositories")
crate_repositories()

To make this happens, some changes needs to be made in rules_rust :
Currently, the crates_repository is generating a defs.bzl file with some http_archive rule generated, with bzlmod, the name of the module is added to the name of the http_archive as expected : the generate_utils is using a default render_config with crate_label_template = "@{repository}__{name}-{version}//:{target}",

This is not usable with bzlmod, due to the canonical name of the repo

To fix:
I can try to add in my project a custom render_config without the "{repository}__", but each project will need to do that.. so :
What is the value to keep {repository} in the rules_rust implementation of the default render_config?

And then list all my dependencies in the use_repo, add also another extension to load the generated defs.bzl file..
Adding an extension, listing all dependencies in use_repo.. if possible i would avoid to do it :)
Do you have any other more elegant idea to achieve this ?

Thanks!
Olivier

@matts1
Copy link
Contributor Author

matts1 commented Jul 20, 2023

@opicaud I've already implemented that. This is just a small part of that PR that I pulled into another PR because the other one was quite big. See #1910.

More specifically, if you look at e199b6a#diff-fe7a82b8bc01fb5cb63b2644f2f11f1fffc9221a622dbfa9af48b20018975486, you'll see:

crate = use_extension("@rules_rust//rust:extensions.bzl", "crate")
crate.from_cargo(
    # This is optional, but if you need crate.annotation, then this is the
    # bzlmod equivalent.
    annotation_files = ["//:annotations.json"],
    cargo_lockfile = "//:Cargo.lock",
    manifests = ["//:Cargo.toml"],
)
use_repo(
    crate,
    # Rename it from <module_name>_crates to crates
    crates = "crates_repository_crates",
)

@csmulhern
Copy link
Contributor

Anything I can do to help push this along? I'd love to get to decent bzlmod support in rules_rust.

@matts1
Copy link
Contributor Author

matts1 commented Aug 2, 2023

I've mostly just been stuck on reviews - I've been waiting for a reviewer to merge this for over a month now. If you know of any process I can go through to expedite the process, then that'd be great, but other than that, I probably don't need anything for now.

@csmulhern
Copy link
Contributor

Maybe @UebelAndre can help with that?

@opicaud
Copy link

opicaud commented Aug 21, 2023

Hey guys,

I think it makes sense to make the effort to merge this : I mean, as for today, this feature is unfortunately not usable : if A depends on B which itself depends on @matts1 rules_rust fork, then you can't build A, because of A is saying Error computing the main repository mapping: The MODULE.bazel file of B declares overrides

So, to be used by A, B must not have any override rules in its MODULE file (rules_rust or something else..)
Whch means, to prevent this in the future, rules_rust needs to be published on BCR, in order to used as bazel_dep instead of *_override and so avoiding this tree dependency issue

I've tried already to rebase the main branch, but with conflict.. i can give it again a try and point out issues

@matts1
Copy link
Contributor Author

matts1 commented Aug 22, 2023

@opicaud While I do want it reviewed, in the meantime, you can use the feature, with the following added to your bazelrc.

common --registry=https://bcr.bazel.build
common --registry=https://raw.githubusercontent.com/matts1/bazel-central-registry/main

Just add the following entry to your MODULE.bazel and it's fine

bazel_dep(name = "rules_rust", version = "0.20.1")

@matts1
Copy link
Contributor Author

matts1 commented Aug 22, 2023

@mobileink
Copy link

mobileink commented Aug 25, 2023

@matts1 Following your example code, I'm getting:

 Updating crates.io index
error: the lock file /var/folders/wz/dx0cgvqx5qn802qmc3d4hcfr0000gr/T/.tmpyzlcRs/Cargo.lock needs to be updated 
but --locked was passed to prevent this
If you want to try to generate the lock file without accessing the network,
 remove the --locked flag and use --offline instead.

If I delete Cargo.lock, I get Error in path: Not a regular file: /Users/gar/obazl/tree-sitter/Cargo.lock

What am I doing wrong? (I don't know much Rust, alas.)

[Update] Ok, I got it working. But I had to rustup update and then cargo update to get past the locking problem. I think this should be unnecessary with Bazel, no? In any case, I can build with a bazel_dep on rule_rust in the current project (@stylites) but when I try to use that project as an external repo I get this error:

.../external/rules_rust~0.20.1/crate_universe/module_extensions/crate.bzl", line 104, column 24, in _crate_impl
		cargo_bazel([ ...
...
Error: Failed to load SplicingManifest

Caused by:
    Failed to parse label from string: @@stylites~1.0.0//:Cargo.toml at line 5 column 138

This may come from the cargo-bazel-splicing-manifest.json file, which has entries like:

 "manifests": {
     "/private/var/tmp/_bazel_gar/a269a6f56ff2403cccdf829695d65824/external/stylites~1.0.0/Cargo.toml": "@@stylites~1.0.0//:Cargo.toml",
...
}

[UPDATE] The error message may come from here:

fn from_str(s: &str) -> Result<Self, Self::Err> {
    let re = Regex::new(r"^(@@?[\w\d\-_\.]*)?/{0,2}([\w\d\-_\./+]+)?(:([\+\w\d\-_\./]+))?$")?;
    let cap = re
        .captures(s)
        .with_context(|| format!("Failed to parse label from string: {s}"))?;

(https://github.com/matts1/rules_rust/blame/e4d947720b633b552a16750e98da18f764fd169b/crate_universe/src/utils/starlark/label.rs#L22-L26)

I don't think that regex handles the tilde?

@matts1
Copy link
Contributor Author

matts1 commented Aug 28, 2023

Cargo update is still required. Cargo update tells rust to update the lockfile. Your error basically says that the Cargo.lock and Cargo.toml files are incompatible with each other (eg. you update crate foo to 2.0.0 but the lockfile still says foo 1.0.0).

The tilde could be the problem, but I would have thought we'd see the problem pop up earlier for all crates, since every repo should have a tilde in it.

@mobileink
Copy link

First, I neglected to say thank you for making this available. I'm bazelizing tree-sitter and was gearing up to finish bazel module support for rules_rust when I found this issue. Whew! So: many thanks.

The tilde could be the problem, but I would have thought we'd see the problem pop up earlier for all crates, since every repo should have a tilde in it.

No, repo strings only have tildes when they are used as external repos. If you build a target like //examples/imp from within the repo that depends on rules_rust (with your modifications), then it works just fine. But in that case the (local) repo strings do not include the version. For example, in my tree-sitters repo, the cargo-bazel-splicing-manifest.json file has:

 "manifests": {
      "/Users/gar/obazl/tree-sitter/Cargo.toml": "@@//:Cargo.toml",
     ...
}

But when I use my tree-sitter repo (module name: stylites) as an external dep (i.e. bazel_dep(name = "stylites", version = "1.0.0"), the the build fails with the parse failure noted in my first message, and the cargo-bazel-splicing-manifest.json shows:

 "manifests": {
      "/private/var/tmp/_bazel_gar/7683f91d463cbaef14ff474a7b07be37/external/stylites~1.0.0/Cargo.toml": "@@stylites~1.0.0//:Cargo.toml",
    ...
}

And since the error message is thrown by the regex application, it seems pretty clear that the regex is wrong.

rust/extensions.bzl Outdated Show resolved Hide resolved
@katre
Copy link
Member

katre commented Sep 27, 2023

Thanks for this. Are there any plans to follow up with support for directly declaring the packages used (as in crate_universe.packages?

It's referred to in the code as build_bazel_apple_support.
This is the same as the @toolchain_<host>_<host> repository. However, we
need to define it seperately because otherwise we'd need to
use_extension on every single toolchain.
@matts1
Copy link
Contributor Author

matts1 commented Oct 5, 2023

@katre Not for now. For now I'm focusing on generating rust packages from Cargo.toml / Cargo.lock files. It shouldn't be difficult to add support for though if someone else wants to do so though - it seems strictly easier than what I'm doing with the cargo files.

@scentini scentini enabled auto-merge (squash) October 25, 2023 14:14
@scentini scentini merged commit 535af8b into bazelbuild:main Oct 25, 2023
3 checks passed
@matts1 matts1 deleted the cargo_bazel_bootstrap branch November 10, 2023 05:50
@illicitonion
Copy link
Collaborator

@matts1 @scentini #2372 is trying to use this in CI, and on macOS it's trying to run a Linux cargo binary. I repro locally.

I don't see anything platform-specific in this PR, but... Any idea why it's picking the wrong cargo for bootstrap?

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.

None yet

9 participants