-
Notifications
You must be signed in to change notification settings - Fork 21
libtest on stable Rust #11
base: master
Are you sure you want to change the base?
Conversation
cc @alexcrichton all CI is suspiciously green on first try 😆 The new build jobs on Travis and Azure appear to be testing the right Rust channels. Once this is merged I'll release 0.0.2 and get compiletest to use it and check that everything works fine, and once that is the case I'll update the libtest version in rust-lang/rust. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I suspect moving to termcolor
may have some fallout on CI, but we can deal with that when we get there.
Can you also do a smoke test using [patch]
to build this in upstream rust-lang/rust? Just to make sure that it builds at least through the first stage. (actually this may be reasonable to add to CI as it's in theory "guaranteed to work")
I think I will try to set this up on CI before merging with rust upstream, but I feel its more important to unbrean clippy in the immediate future. |
So I've ran into a couple of problems while updating upstream that go beyond cosmetic changes:
I am not sure how to fix that @alexcrichton, but we can always fix that later and when that's done release |
I think it'd probably be best to decouple whatever clippy/miri/others need from this PR? I don't know what's going on with the libc/termcolor "showing up once" problem, but it's something I feared would happen and is why it was never previously pursued to remove the libterm dependency. I don't currently have the time to dig in and debug, so if fixes need to go in otherwise it may be best to get those first. |
Can we speed up merging this? Clippy CI is broken, and this affects clippy releases in nightly. |
Either that or undo the changes to libtest in rustc until we have a better overall story for this here. |
src/lib.rs
Outdated
} | ||
#[cfg(any(not(feature = "unstable"), stage0))] | ||
{ | ||
unsafe { std::ptr::read_volatile(&x as *const T) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm, doesn't x
's destructor execute immediately after this line? I think a mem::forget or something is needed after the read_volatile
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, care submitting a PR? Otherwise, I'll fix that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't submit PR to PR, i think... You can fix that yourself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can submit a PR to the clippy_ci branch on gnzlbg's fork
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gnzlbg Are you still planning on working on this? |
@XAMPPRocky I believe this work is blocked. Let me try to summarize this:
|
@crlf0710 Thank you for summarising and clarifying the current state of this work! |
This PR makes libtest work on stable Rust by default. The stable and beta release channels are now tested on CI, and the crate was migrated to use the stable
termcolor
crate per the recommendation in #2 .This PR adds:
an
unstable
cargo feature that is disabled by default and can be optionally enabled to allowlibtest
to make use of some nightly-only features.the
compiletest-rs
crate as an integration test (cc @Manishearth ). Note: this test fails for now because thecompiletest-rs
needs to be updated tolibtest 0.0.2
for it to work, which hasn't happened yet. It is therefore allowed to fail.