rustbuild: Allow quick testing of libstd and libcore at stage0 #50466
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This enables `./x.py test --stage 0 src/libstd --no-doc` and ensures the stage2-rustc and rustdoc need to be built.
All other tests of libcore reside in the tests/ directory, too. Apparently the tests of `time.rs` weren't run before, at least not by `x.py test src/libcore`.
|
r? @TimNN (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
cc @Zoxc #49119. I'm not sure what is your plan in that PR, but I'd like to ensure cc @LukasKalbertodt #50364. I've copy your two commits fixing libcore tests here. |
|
This looks good to me, but I'm not familiar enough with rustbuild to properly review this, so |
|
Could the magic |
src/libcore/lib.rs
Outdated
| @@ -50,6 +50,7 @@ | |||
|
|
|||
| // Since libcore defines many fundamental lang items, all tests live in a | |||
| // separate crate, libcoretest, to avoid bizarre issues. | |||
| #![cfg(not(test))] | |||
There was a problem hiding this comment.
This feels odd; presumably independent of whether we are running tests we would expect core to exist and define everything -- or am I misreading what this does?
There was a problem hiding this comment.
The problem is when we are testing the library, we will build an executable and link to libtest. Now, this executable defines some lang items, and at the same time libtest imports the non-testing libcore which also defines some lang items. This will cause the E0152 "duplicate lang item found" error. I'm not sure why this is not discovered before, maybe no one tried to ./x.py test src/libcore? 🙃
Reduced test case:
#![cfg(not(test))] // <-- comment out to cause E0152 when `cargo test`.
#![feature(no_core, lang_items)]
#![no_core]
/// ```
///
/// ```
#[lang = "sized"]
pub trait Sized {}In cargo test, the doc tests are still discovered and executed even with this cfg. I assume this is the same for rustbuild, though if it turns out to be wrong we could change the Edit: Yes they are executed.cfg to any(dox, not(test)).
I've updated this comment to explain the attribute.
src/bootstrap/lib.rs
Outdated
| @@ -210,6 +210,16 @@ pub struct Compiler { | |||
| host: Interned<String>, | |||
| } | |||
|
|
|||
| #[derive(PartialEq, Eq, Copy, Clone, Debug)] | |||
| pub enum DocTestsOption { | |||
There was a problem hiding this comment.
Can we rename this to DocTests? I think it might have uses outside just being passed on the command line...
This should probably go in the rustc-guide testing page. |
|
@retep998 I've added it to |
|
I think I'm almost ready to sign off on this, but the problem I see with the If I am, we might be able to work around this by making specifically the lang items defined by core |
|
@Mark-Simulacrum libcore will be tested through the library in I'm not going to modify
We see indeed it is testing the modified libcore. |
|
That is surprising to me -- I thought that we linked against a library compiled with However, since it does seem to work, @bors r+ -- can you make a PR or file an issue against rustc-guide too? @rust-lang/libs -- this functionality is likely of interest and use to all of you |
|
📌 Commit 05af55b has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 05af55b with merge 449c634747ab60600155dd81d728bc7f891a056c... |
|
💔 Test failed - status-appveyor |
bors
added
S-waiting-on-review
|
@bors retry |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
May 6, 2018
…lacrum rustbuild: Allow quick testing of libstd and libcore at stage0 This PR implemented two features: 1. Added a `--no-doc` flag to allow testing a crate *without* doc tests. In this mode, we don't need to build rustdoc, and thus we can skip building the stage2 compiler. (Ideally stage0 test should use the bootstrap rustdoc, but I don't want to mess up the core builder logic here) 2. Moved all libcore tests externally and added a tidy test to ensure we don't accidentally add `#[test]` into libcore. After this PR, one could run `./x.py test --stage 0 --no-doc src/libstd` to test `libstd` without building the compiler, thus enables us to quickly test new library features.
|
Is this incompatible with #49119 ? |
|
@SimonSapin It is not. |
|
@SimonSapin It still runs |
|
☀️ Test successful - status-appveyor, status-travis |
This was referenced May 6, 2018
Merged
|
I'm not sure if I should open a new issue for this: I tried the new command, and it didn't work for me in some situations. When I execute Note that I can simply run EDIT: But thanks for this! It saves me so much time already <3 |
|
@LukasKalbertodt Yes please file a new issue. As a workaround, you could just delete that |
3 tasks
|
I've just tried using this with Is this expected to work only with core and std, but not alloc? |
|
@nikic Sorry I hadn't tested diff --git a/src/liballoc/alloc.rs b/src/liballoc/alloc.rs
index f59c9f7fd6..7e31ee664c 100644
--- a/src/liballoc/alloc.rs
+++ b/src/liballoc/alloc.rs
@@ -152,7 +152,7 @@ unsafe fn exchange_malloc(size: usize, align: usize) -> *mut u8 {
}
}
-#[cfg(stage0)]
+#[cfg(all(stage0, not(test)))]
#[lang = "box_free"]
#[inline]
unsafe fn old_box_free<T: ?Sized>(ptr: *mut T) {Since we will be releasing a new Rust version next week, this stage0 item will soon be removed, so I won't create a dedicated PR to fix it (maybe together with #50481 if the solution is found). |
bors
added a commit
that referenced
this pull request
May 26, 2018
…=KodrAus
Improve `Debug` impl of `time::Duration`
Hi there!
For a long time now, I was getting annoyed by the derived `Debug` impl of `Duration`. Usually, I use `Duration` to either do quick'n'dirty benchmarking or measuring the time of some operation in general. The output of the derived Debug impl is hard to parse for humans: is { secs: 0, nanos: 968360102 } or { secs: 0, nanos 98507324 } longer?
So after running into the annoyance several times (sometimes building my own function to print the Duration properly), I decided to tackle this. Now the output looks like this:
```
Duration::new(1, 0) => 1s
Duration::new(1, 1) => 1.000000001s
Duration::new(1, 300) => 1.0000003s
Duration::new(1, 4000) => 1.000004s
Duration::new(1, 600000) => 1.0006s
Duration::new(1, 7000000) => 1.007s
Duration::new(0, 0) => 0ns
Duration::new(0, 1) => 1ns
Duration::new(0, 300) => 300ns
Duration::new(0, 4001) => 4.001µs
Duration::new(0, 600300) => 600.3µs
Duration::new(0, 7000000) => 7ms
```
Note that I implemented the formatting manually and didn't use floats. No information is "lost" when printing. So `Duration::new(123_456_789_000, 900_000_001)` prints as `123456789000.900000001s`.
~~This is not yet finished~~, but I wanted to open the PR now already in order to get some feedback (maybe everyone likes the derived impl).
### Still ToDo:
- [x] Respect precision ~~and width~~ parameter of the formatter (see [this comment](#50364 (comment)))
### Alternatives/Decisions
- Should large durations displayed in minutes, hours, days, ...? For now, I decided not to because the current formatting is close the how a `Duration` is stored. From this new `Debug` output, you can still easily see what the values of `secs` and `nanos` are. A formatting like `3h 27m 12s 9ms` might be more appropriate for a `Display` impl?
- Should this rather be a `Display` impl and should `Debug` be derived? Maybe this formatting is too fancy for `Debug`? In my opinion it's not and, as already mentioned, from the current format one can still very easily determine the values for `secs` and `nanos`.
- Whitespace between the number and the unit?
### Notes for reviewers
- ~~The combined diff sucks. Rather review both commits individually.~~
- ~~In the unit test, I am building my own type implementing `fmt::Write` to test the output. Maybe there is already something like that which I can use?~~
- My `Debug` impl block is marked as `#[stable(...)]`... but that's fine since the derived Debug impl was stable already, right?
---
~~Apart from the main change, I moved all `time` unit tests into the `tests` directory. All other `libcore` tests are there, so I guess it was simply an oversight. Prior to this change, the `time` tests weren't run, so I guess this is kind of a bug fix. If my `Debug` impl is rejected, I can of course just send the fix as PR.~~ (this was already merged in #50466)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implemented two features:
Added a
--no-docflag to allow testing a crate without doc tests. In this mode, we don't need to build rustdoc, and thus we can skip building the stage2 compiler. (Ideally stage0 test should use the bootstrap rustdoc, but I don't want to mess up the core builder logic here)Moved all libcore tests externally and added a tidy test to ensure we don't accidentally add
#[test]into libcore.After this PR, one could run
./x.py test --stage 0 --no-doc src/libstdto testlibstdwithout building the compiler, thus enables us to quickly test new library features.