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

Establish CARGO_MANIFEST_DIR #180

Merged
merged 1 commit into from
Jan 4, 2019
Merged

Conversation

GregBowyer
Copy link
Contributor

Several popular crates, as well as some source code has a habit of using env!("CARGO_MANIFEST_DIR") to find files in a crate. This causes things like google/cargo-raze#71.

Following @acmcarther suggestions this is an attempt at solution 3, which appears to handle (ab)uses of CARGO_MANIFEST_DIR I have tested (tera, askuma).

@mfarrugi
Copy link
Collaborator

To be clear, libraries that use this need to declare whatever additional files as data dependencies, and then everything Just Works?

Copy link
Collaborator

@acmcarther acmcarther left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I'm refreshed to hear that one of my large comments was used to produce a fix!

I think I'd like to see an example or test added for this -- if only to guarantee to later readers that the feature worked in this PR. I can understand if that might be onerous.... chiefly in having to either vendor deps for Pest, or produce a minimum example from Pest's source. I happen to prefer the second option if no other presents itself.

rust/private/rustc.bzl Outdated Show resolved Hide resolved
See google/cargo-raze#71 (comment) for the rationale as
to why

In brief:

Certain rust build processes expect to find files from the environment variable
`$CARGO_MANIFEST_DIR`. Examples of this include pest, tera, asakuma.

The compiler and by extension proc-macros see the current working directory as the Bazel exec
root. Therefore, in order to fix this without an upstream code change, we have to set
`$CARGO_MANIFEST_DIR`.

As such we attempt to infer `$CARGO_MANIFEST_DIR`.
Inference cannot be derived from `attr.crate_root`, as this points at a source file which may or
may not follow the `src/lib.rs` convention. As such we use `ctx.build_file_path` mapped into the
`exec_root`. Since we cannot (seemingly) get the `exec_root` from skylark, we cheat a little
and use `$(pwd)` which resolves the `exec_root` at action execution time.

Fixes: google/cargo-raze#71
@GregBowyer
Copy link
Contributor Author

@mfarrugi Yes, data attributes will still need to be set. All this does is bind CARGO_MANIFEST_DIR to the crate root so that env!("CARGO_MANIFEST_DIR") finds this path.

@acmcarther rather than vendor pest, which would only work for this snapshot / point in time, would a simple rust file that uses CARGO_MANIFEST_DIR be suitable? If so thats easy and a test has been added.

I think for pest / tera maybe we should add a test over in cargo-raze as a smoke test too? Thoughts?

@GregBowyer
Copy link
Contributor Author

@acmcarther ... in fact if anything maybe with this we could solve google/cargo-raze#61 which would also function as a fairly solid test

@mfarrugi
Copy link
Collaborator

mfarrugi commented Jan 4, 2019

I think the test added here is sufficient (not sure if it was added before or after the comment). @acmcarther wdyt?

Copy link
Collaborator

@acmcarther acmcarther left a comment

Choose a reason for hiding this comment

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

Thanks for your patience, test does look good to me.

@acmcarther acmcarther merged commit f32695d into bazelbuild:master Jan 4, 2019
@GregBowyer
Copy link
Contributor Author

@mfarrugi added after for your reference.

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

Successfully merging this pull request may close these issues.

None yet

4 participants