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

Add the CARGO_CRATE_NAME env var #537

Closed
wants to merge 1 commit into from
Closed

Conversation

arlyon
Copy link

@arlyon arlyon commented Dec 16, 2020

See #536 for more discussion.

@google-cla google-cla bot added the cla: yes label Dec 16, 2020
Copy link
Collaborator

@illicitonion illicitonion 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 contribution! I agree that we should set this env var, but I think we should maybe set it to a slightly different value - let me know what you think :)

To see cargo's behaviour:

$ cargo init crate-with-dashes
$ cd crate-with-dashes
$ cat >src/main.rs <<EOF
fn main() {
    println!("CARGO_CRATE_NAME: {}", env!("CARGO_CRATE_NAME"));
    println!("CARGO_PKG_NAME: {}", env!("CARGO_PKG_NAME"));
}
EOF
$ cargo run --quiet
CARGO_CRATE_NAME: crate_with_dashes
CARGO_PKG_NAME: crate-with-dashes

cargo/cargo_build_script.bzl Show resolved Hide resolved
@arlyon arlyon force-pushed the patch-1 branch 2 times, most recently from 107eeea to 883a04c Compare December 16, 2020 22:40
@illicitonion
Copy link
Collaborator

Sorry for the delay!

Unfortunately this PR now goes the wrong way around: CARGO_PKG_NAME should always be set based on the name attribute, and crate_name should replace -s with _s. The CARGO_PKG_NAME may contain _s, whereas right now this PR replaces all _s with -.

Base automatically changed from master to main January 28, 2021 20:47
@hlopko
Copy link
Member

hlopko commented Feb 16, 2021

Friendly ping from a bywalker :)

@arlyon
Copy link
Author

arlyon commented Feb 18, 2021

Thanks for the ping, this fell of my todo list :) I will push changes asap. Am I right in saying that 'crate_name' in rules rust actually refers to the pkg name then? ie. rules_rust uses the name an Cargo.toml rather than the name in code.

It feels inconsistent so I want to get it right:

ctx.attr.crate_name == "my-cool_crate"

pkg_name = ctx.attr.crate_name
crate_name = pkg_name.replace("_", "-")

pkg_name == "my-cool_crate"
crate_name == "my_cool_crate"

@UebelAndre
Copy link
Collaborator

Friendly drive-by ping, any updates here? 😄

@arlyon
Copy link
Author

arlyon commented Mar 18, 2021

We're not far off, but if the crate name in rules_rust is in fact the pkg_name then that differenciation should be reflected in the properties in the attributes and we should rename it. I'm not sure about the implications of this though so some feedback would be helpful.

@illicitonion
Copy link
Collaborator

We're not far off, but if the crate name in rules_rust is in fact the pkg_name then that differenciation should be reflected in the properties in the attributes and we should rename it. I'm not sure about the implications of this though so some feedback would be helpful.

That's a really good point! And I think we can answer it with my favourite answer of all: Let's delete the attribute!

As far as I can tell, we currently use the crate_name attribute in exactly three places:

  1. We use it to set CARGO_PKG_NAME
  2. We uppercase it and replace -s with _s to set CARGO_FEATURE_${CRATE_NAME}=1 env vars per feature
  3. We use it for some legacy fallback behaviour to guess what the links attribute should've been before we had the links attribute - Set links attribute google/cargo-raze#400 will allow us to delete the fallback behaviour.

(Worthy of note, we don't use it for the thing it was originally added for in #341, which was to set DEP_ env vars, they now use the links attribute, except where that fallback is used).

So I'd be tempted to delete the crate_name attribute entirely, and just treat the name attribute like the thing we'd see in a package.name of a Cargo.toml. If we did that, we'd (after removing a trailing _build_script if present) set CARGO_PKG_NAME to exactly ctx.attr.name, set CARGO_CRATE_NAME to the -s to _s version, set CARGO_FEATURE_${CRATE_NAME} using the uppercased version of CARGO_CRATE_NAME, and have removed an unnecessary attribute on the way.

@hlopko
Copy link
Member

hlopko commented Mar 18, 2021

SGTM, I'm a big fan of deleting and simplifying code, let's do this!

@arlyon
Copy link
Author

arlyon commented Mar 30, 2021

Closing after a more comprehensive solution here #643 thanks all!

@arlyon arlyon closed this Mar 30, 2021
@arlyon arlyon deleted the patch-1 branch March 30, 2021 15:23
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