google / cargo-raze

Generate Bazel BUILD from Cargo dependencies!
Apache License 2.0
477 stars 104 forks source link

Clearer documentation of `workspace_path` configuration #335

Open cceckman opened 3 years ago

cceckman commented 3 years ago

Minimal (maybe) demo in https://github.com/cceckman/raze-bug.

TL;DR It seems that "Bazel workspace root" == "Cargo workspace root" doesn't work; cargo raze generates a label that begins ///, which is not valid in Bazel.

Is this: a) working as intended; b) a bug; c) not actually the case, and instead I've done something wrong? (Entirely possible! :-) )


This setup in https://github.com/cceckman/raze-bug is:

The error:

∵ bazel test //alib:alib_test
ERROR: Traceback (most recent call last):
        File "/home/cceckman/r/github.com/cceckman/raze/WORKSPACE.bazel", line 22, column 25, in <toplevel>
                raze_fetch_remote_crates()
        File "/home/cceckman/r/github.com/cceckman/raze/crates.bzl", line 21, column 27, in raze_fetch_remote_crates
                build_file = Label("///remote:BUILD.derivative-2.1.1.bazel"),
Error in Label: Illegal absolute label syntax: ///remote:BUILD.derivative-2.1.1.bazel
ERROR: error loading package 'external': Package 'external' contains errors
INFO: Elapsed time: 0.114s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (0 packages loaded)
FAILED: Build did NOT complete successfully (0 packages loaded)
∴ 1

(Same error from bazel build //alib:alib.)

Versions used in VERSIONS file there - it's HEAD of rules_rust and 0.8.0 of cargo raze.

If I'm reading right- and I may not be - this line comes from remote_crates.bzl.template; here at the version of cargo raze used in my test, but still here at HEAD. The formatting there seems to assume that the relative path is not "the root of the repository".

The "smoke tests" don't seem to exercise this condition:

∵ pwd; ls
/home/cceckman/r/github.com/google/cargo-raze/smoke_test
README.md  remote  tests  vendored
∴ 0 cargo-raze:master…smoke_test
∵ grep -Rh 'workspace_path'
workspace_path = "//remote/no_deps/cargo"
workspace_path = "//remote/cargo_workspace/cargo"
workspace_path = "//remote/binary_dependencies/cargo"
workspace_path = "//remote/complicated_cargo_library/cargo"
workspace_path = "//remote/non_cratesio/cargo"
workspace_path = "//vendored/hello_cargo_library/cargo"
workspace_path = "//vendored/cargo_workspace/cargo"
workspace_path = "//vendored/complicated_cargo_library/cargo"
workspace_path = "//vendored/non_cratesio_library/cargo"
∴ 0 cargo-raze:master…smoke_test

So:

1) Is this analysis correct (this is a bug and the cause)? (If not - please point me in the right direction and thanks for the help :-) ) 2) Is there way to add a test for this? (I haven't looked much at the test infrastructure here.) 3) What's the right fix for this?

cceckman commented 3 years ago

(XRef #262 / @UebelAndre , as that seems to be the main FR / contributed for workspace support - thanks for your efforts so far!)

cceckman commented 3 years ago

OK; yeah, it's user error. Fixed (kind of) in https://github.com/cceckman/raze-bug/tree/fixed.

I had read "workspace_path" as "this Cargo.toml's relative path to the root"; when it's actually "where to put (some of) the generated files".

I'm not in the right circadian cycle to think about doc updates for this, but may be able to tomorrow. Sorry for the noise >.<

UebelAndre commented 3 years ago

Yeah, workspace_path is "where to put the outputs relative to the bazel workspace root. It's not limited to the path to the Cargo.toml file. Maybe in addition to updating the docs, the naming of it can be updated too. workspace_path doesn't feel like it communicates enough. Any suggestions (question open to everyone 😄)?

cceckman commented 3 years ago

output_path, maybe?