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

Don't apply name_to_crate_name to test binary names. #670

Merged
merged 3 commits into from
Mar 31, 2021

Conversation

martinboehme
Copy link
Collaborator

Normal rust_binary targets don't do this, so for consistency, don't do it in tests either.

An additional motivation for this change is that I'm trying to whittle down the number of uses of the name_to_crate_name function, as it doesn't take a possible crate_name attribute into account. For context, see also the changes and discussion in

#645

Normal `rust_binary` targets don't do this, so for consistency, don't do
it in tests either.

An additional motivation for this change is that I'm trying to whittle down
the number of uses of the `name_to_crate_name` function, as it doesn't
take a possible `crate_name` attribute into account. For context, see
also the changes and discussion in

bazelbuild#645
@google-cla google-cla bot added the cla: yes label Mar 31, 2021
@martinboehme
Copy link
Collaborator Author

@illicitonion I see now that you specifically introduced the hyphen-to-underscore conversion in #528. I'm not quite clear on why it was required at the time though... can you explain?

@martinboehme
Copy link
Collaborator Author

@illicitonion At any rate, I've run bazel test @examples//hello_lib/..., and everything passes (including the hello-lib-test that you renamed to contain dashes in the name in #528), so whatever the original problem was, it appears it's fixed now?

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.

Hmm...

On the one hand, some other change must have compensated for the change I added for the failing test, because the tests are passing.

On the other hand, cargo does do this replacement for test binaries (notice the "Running" line):

% cargo init test-with-hyphens
     Created binary (application) package
% cd test-with-hyphens
% cargo test
   Compiling test-with-hyphens v0.1.0 (/Users/dwh/tmp/test-with-hyphens)
    Finished test [unoptimized + debuginfo] target(s) in 0.72s
     Running target/debug/deps/test_with_hyphens-17b0d88f95f3c1f4

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

But "how cargo happens to name intermediate outputs" isn't super important, people shouldn't be relying on it (and if they are, we should add tests covering the contract we offer).

I guess we'll find out if something fails, and can always switch back if we need... WDYT?

@martinboehme
Copy link
Collaborator Author

Hmm...

On the one hand, some other change must have compensated for the change I added for the failing test, because the tests are passing.

On the other hand, cargo does do this replacement for test binaries (notice the "Running" line):

% cargo init test-with-hyphens
     Created binary (application) package
% cd test-with-hyphens
% cargo test
   Compiling test-with-hyphens v0.1.0 (/Users/dwh/tmp/test-with-hyphens)
    Finished test [unoptimized + debuginfo] target(s) in 0.72s
     Running target/debug/deps/test_with_hyphens-17b0d88f95f3c1f4

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

Interesting...

But "how cargo happens to name intermediate outputs" isn't super important, people shouldn't be relying on it (and if they are, we should add tests covering the contract we offer).

I guess we'll find out if something fails, and can always switch back if we need... WDYT?

I think there are two competing concerns here: a) What does Cargo do, and b) What would Bazel typically do?

Bazel would typically name the output files for the Bazel target name, and we already do this for rust_binary rules, and I think it makes more sense to be consistent with Bazel's conventions rather than Cargo's conventions because we are, after all, executing a Bazel build rule.

So if you're happy with this, I'm happy to try it out. I don't want to force this on you though -- nothing I'm doing actually needs this change, I just noticed in the course of making other changes and thought it would be nice to make rust_test more consistent with rust_binary here. And so I would have not problem reverting this change if it turns out to be a problem.

All of this is a long-winded way of saying: If you're happy with this, please merge (I don't have write access). If you'd rather hold off on this, I'm fine with that too.

@illicitonion illicitonion merged commit feefdd4 into bazelbuild:main Mar 31, 2021
yagehu pushed a commit to yagehu/rules_rust that referenced this pull request Apr 23, 2021
Normal `rust_binary` targets don't do this, so for consistency, don't do it in tests either.

An additional motivation for this change is that I'm trying to whittle down the number of uses of the `name_to_crate_name` function, as it doesn't take a possible `crate_name` attribute into account. For context, see also the changes and discussion in

bazelbuild#645
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

3 participants