Skip to content

*: remove crate_{name,type} attributes #43994

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

Merged
merged 3 commits into from
Aug 26, 2017
Merged

Conversation

tamird
Copy link
Contributor

@tamird tamird commented Aug 19, 2017

Fixes #41701.

r? @arielb1

@tamird
Copy link
Contributor Author

tamird commented Aug 20, 2017

Note sure what to make of these test failures:

Testing libtest stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
   Compiling term v0.0.0 (file:///checkout/src/libterm)
   Compiling test v0.0.0 (file:///checkout/src/libtest)
error[E0308]: mismatched types
   --> /checkout/src/libtest/stats.rs:887:5
    |
887 | /     pub fn sum_three_items(b: &mut Bencher) {
888 | |         b.iter(|| {
889 | |             [1e20f64, 1.5f64, -1e20f64].sum();
890 | |         })
891 | |     }
    | |_____^ expected struct `__test::test::Bencher`, found struct `Bencher`
    |
    = note: expected type `fn(&mut __test::test::Bencher)`
               found type `fn(&mut Bencher) {stats::bench::sum_three_items}`

error[E0308]: mismatched types
   --> /checkout/src/libtest/stats.rs:893:5
    |
893 | /     pub fn sum_many_f64(b: &mut Bencher) {
894 | |         let nums = [-1e30f64, 1e60, 1e30, 1.0, -1e60];
895 | |         let v = (0..500).map(|i| nums[i % 5]).collect::<Vec<_>>();
896 | |
...   |
899 | |         })
900 | |     }
    | |_____^ expected struct `__test::test::Bencher`, found struct `Bencher`
    |
    = note: expected type `fn(&mut __test::test::Bencher)`
               found type `fn(&mut Bencher) {stats::bench::sum_many_f64}`

error[E0308]: mismatched types
   --> /checkout/src/libtest/stats.rs:903:5
    |
903 |     pub fn no_iter(_: &mut Bencher) {}
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `__test::test::Bencher`, found struct `Bencher`
    |
    = note: expected type `fn(&mut __test::test::Bencher)`
               found type `fn(&mut Bencher) {stats::bench::no_iter}`

error: aborting due to 3 previous errors

error: Could not compile `test`.
warning: build failed, waiting for other jobs to finish...
error: build failed


command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "-j" "4" "--target" "x86_64-unknown-linux-gnu" "--release" "--locked" "--color" "always" "--manifest-path" "/checkout/src/libtest/Cargo.toml" "-p" "test:0.0.0" "-p" "term:0.0.0" "--" "--quiet"
expected success, got: exit code: 101


failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
Build completed unsuccessfully in 0:23:20
Makefile:54: recipe for target 'check' failed
make: *** [check] Error 1

@arielb1
Copy link
Contributor

arielb1 commented Aug 20, 2017

The problem is this function, which redirects tests of crates with a libtest attribute to their own test infrastructure, instead of the builtin libtest.

rust/src/libsyntax/test.rs

Lines 611 to 617 in 6f4ab94

fn is_test_crate(krate: &ast::Crate) -> bool {
match attr::find_crate_name(&krate.attrs) {
Some(s) if "test" == s.as_str() => true,
_ => false
}
}

@tamird tamird force-pushed the remove-attributes branch from 8f64c56 to 851d094 Compare August 20, 2017 16:04
@@ -85,7 +85,7 @@ pub fn modify_for_testing(sess: &ParseSess,
"reexport_test_harness_main");

if should_test {
generate_test_harness(sess, resolver, reexport_test_harness_main, krate, span_diagnostic)
generate_test_harness(sess, crate_name, resolver, reexport_test_harness_main, krate, span_diagnostic)
Copy link
Member

Choose a reason for hiding this comment

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

This line is too long.

[00:03:11] tidy error: /checkout/src/libsyntax/test.rs:88: line longer than 100 chars

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, fixed.

@tamird tamird force-pushed the remove-attributes branch from 851d094 to 07fc66c Compare August 20, 2017 16:36
@tamird
Copy link
Contributor Author

tamird commented Aug 20, 2017

Thanks @arielb1! This is green.

@arielb1
Copy link
Contributor

arielb1 commented Aug 20, 2017

These sort of changes are more of @alexcrichton's domain.

@arielb1
Copy link
Contributor

arielb1 commented Aug 20, 2017

r? @alexcrichton

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Collaborator

bors commented Aug 22, 2017

📌 Commit 07fc66c has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Aug 22, 2017

⌛ Testing commit 07fc66c2583ded22f061ef7eb643e62edef7a707 with merge 86ca4b4773693d5f666a96fffefe1f0c95d226ff...

@bors
Copy link
Collaborator

bors commented Aug 22, 2017

💔 Test failed - status-travis

@kennytm
Copy link
Member

kennytm commented Aug 22, 2017

x86_64-gnu-aux could not build stage2-cargo, the crate test not found.

[01:12:33] error[E0432]: unresolved import `test`
[01:12:33] 
[01:12:34] error: aborting due to previous error
[01:12:34] 
[01:12:34] error: Could not compile `cargo`.
[01:12:34] warning: build failed, waiting for other jobs to finish...
[01:12:58] error: build failed

@aidanhs aidanhs added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 23, 2017
@tamird
Copy link
Contributor Author

tamird commented Aug 24, 2017

I'm guessing that now that libtest doesn't have the name attribute, something's different in the resulting rlib (or dylib, on my mac). @alexcrichton any guidance? FWIW, it does seem like faa6769 is what broke this (and yet it is necessary to solve the earlier build problem).

@alexcrichton
Copy link
Member

Looks like the bug here is that tests/test.rs is being compiled in Cargo which is failing to compile with the new handling. You may wish to just leave the test harness modification unchanged and leave the #![crate_name = "test"] annotation there. Definitely a hack, but should keep things working hopefully

@tamird
Copy link
Contributor Author

tamird commented Aug 24, 2017

@alexcrichton I don't follow: this failure occurs even without the mass removal of attributes (i.e. in the second commit).

@alexcrichton
Copy link
Member

I'd recommend just backing out the modifications to libsyntax/test.rs. When doing that I'd add back #![crate_name] to src/libtest/lib.rs and that I believe should pass tests.

@tamird
Copy link
Contributor Author

tamird commented Aug 25, 2017 via email

@alexcrichton
Copy link
Member

The test harness acts differently based on its calculated value of is_test_crate. Today is_test_crate is true for libtest and is_test_crate is false for Cargo's unit test. With the logic you've done in this PR they both calculate to true. Presumably Cargo's unit test doens't work with is_test_crate being true.

tamird added 2 commits August 25, 2017 16:01
The value of this field is meant to indicate whether or not the
crate is rustc's libtest itself - not whether or not it is a test
crate generally.
@tamird tamird force-pushed the remove-attributes branch from 07fc66c to dccf50d Compare August 25, 2017 20:14
@tamird tamird force-pushed the remove-attributes branch from dccf50d to b3f50ca Compare August 25, 2017 20:18
@tamird
Copy link
Contributor Author

tamird commented Aug 25, 2017

@alexcrichton OK, done.

I don't quite understand this machinery that does specific-to-libtest stuff, but I've go ahead and documented some of this sorcery.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Collaborator

bors commented Aug 25, 2017

📌 Commit b3f50ca has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Aug 25, 2017

⌛ Testing commit b3f50ca with merge 83fcd4d...

bors added a commit that referenced this pull request Aug 25, 2017
*: remove crate_{name,type} attributes

Fixes #41701.

r? @arielb1
@bors
Copy link
Collaborator

bors commented Aug 26, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 83fcd4d to master...

@bors bors merged commit b3f50ca into rust-lang:master Aug 26, 2017
@tamird tamird deleted the remove-attributes branch September 8, 2017 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants