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

Fixed the need for cargo on the host when running //:raze #388

Merged
merged 1 commit into from Feb 24, 2021
Merged

Fixed the need for cargo on the host when running //:raze #388

merged 1 commit into from Feb 24, 2021

Conversation

UebelAndre
Copy link
Collaborator

I forgot that I still have cargo installed on my machine when testing this scenario. The //:raze target should be a wrapper script that explicitly sets the needed environment variables for running cargo-raze without installing cargo components on the host

With the following diff:

diff --git a/examples/WORKSPACE b/examples/WORKSPACE
index 327a21ef..bf138d0c 100644
--- a/examples/WORKSPACE
+++ b/examples/WORKSPACE
@@ -19,3 +19,12 @@ rust_repositories()
 load("//:repositories.bzl", "repositories")

 repositories()
+
+local_repository(
+    name = "cargo_raze",
+    path = "..",
+)
+load("@cargo_raze//:repositories.bzl", "cargo_raze_repositories")
+cargo_raze_repositories()
+load("@cargo_raze//:transitive_deps.bzl", "cargo_raze_transitive_deps")

I'm still able to run raze in another workspace

examples % bazel run @cargo_raze//:raze -- --help
INFO: Invocation ID: d2c294cd-d33d-4df6-b66e-1fbb7a6b49e5
INFO: Analyzed target @cargo_raze//:raze (3 packages loaded, 372 targets configured).
INFO: Found 1 target...
Target @cargo_raze//tools:raze up-to-date:
  bazel-bin/external/cargo_raze/tools/raze
INFO: Elapsed time: 4.432s, Critical Path: 0.04s
INFO: 1 process: 1 internal.
INFO: Build completed successfully, 1 total action
INFO: Build completed successfully, 1 total action
Generate BUILD files for your pre-vendored Cargo dependencies.

Usage:
    cargo-raze (-h | --help)
    cargo-raze (-V | --version)
    cargo-raze [--verbose] [--quiet] [--color=<WHEN>] [--dryrun] [--cargo-bin-path=<PATH>]
               [--manifest-path=<PATH>] [--output=<PATH>] [--generate-lockfile]

Options:
    -h, --help                          Print this message
    -V, --version                       Print version info and exit
    -v, --verbose                       Use verbose output
    -q, --quiet                         No output printed to stdout
    --color=<WHEN>                      Coloring: auto, always, never
    -d, --dryrun                        Do not emit any files
    --cargo-bin-path=<PATH>             Path to the cargo binary to be used for loading workspace metadata
    --manifest-path=<PATH>              Path to the Cargo.toml file to generate BUILD files for
    --output=<PATH>                     Path to output the generated into.
    --generate-lockfile                 Force a new `Cargo.raze.lock` file to be generated

@UebelAndre
Copy link
Collaborator Author

@acmcarther If you could take a quick peak at this, it makes #384 correct 😅

Base automatically changed from master to main February 24, 2021 20:03
)

sh_binary(
name = "raze",
Copy link
Member

Choose a reason for hiding this comment

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

This is not compatible with windows and probably shouldn't be the default "cargo-raze" target.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What makes it incompatible? I thought Bazel expected windows users to have bash installed.

Copy link
Member

Choose a reason for hiding this comment

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

@acmcarther
Copy link
Member

I don't really feel strongly about this, but I suspect this is a step backward for window support.

I'm not sure what doing this properly would look like.

@UebelAndre
Copy link
Collaborator Author

I unfortunately don't have a windows machine to try this out on but am happy to address any concerns if this doesn't work. The correct solution is likely a custom rule that outputs a batch file but I'm not sure

@acmcarther
Copy link
Member

I guess we'll be able to iterate on this once the windows presubmit targets are restored.

(FYI: you should be able to see the build kite builds now)

@acmcarther acmcarther merged commit 82fce72 into google:main Feb 24, 2021
@UebelAndre
Copy link
Collaborator Author

(FYI: you should be able to see the build kite builds now)

I can, thank you! Windows dev is just insanely time consuming since I don't know what I'm doing and can only test in the CI builds 😅

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

2 participants