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 support for compiler tests #90

Merged
merged 3 commits into from Nov 12, 2017

Conversation

Projects
None yet
3 participants
@fuine
Copy link
Contributor

fuine commented Nov 9, 2017

This is an initial version of compiler tests. So far it should work on travis, appveyor is a bit harder to achieve, because there's no simple way to add testing with specified features only on nightly. Please don't merge yet, because I want to at least try to make them run on appveyor as well.

Moreover due to the design of compiletest crate and its implementation details there isn't any clear way to annotate warnings/errors in lines that are being expanded by the macro, but rather we are forced to use error-pattern general mechanism. I will document that in code later on, just wanted to highlight it here.

@@ -17,9 +17,14 @@ categories = [ "no-std", "rust-patterns" ]
version = "0.4"
optional = true

[dependencies.compiletest_rs]

This comment has been minimized.

@KodrAus

KodrAus Nov 9, 2017

Contributor

Could this be a dev-dependency instead?

This comment has been minimized.

@fuine

fuine Nov 10, 2017

Author Contributor

I'm afraid it can't at the moment - rust-lang/cargo#1596 and it needs to be optional, because, due to language features, it simply doesn't build on stable/beta

This comment has been minimized.

@kami881

@fuine fuine force-pushed the fuine:compiletest branch 2 times, most recently from 1ca6c71 to 60e11d8 Nov 10, 2017

@fuine

This comment has been minimized.

Copy link
Contributor Author

fuine commented Nov 10, 2017

@KodrAus could you take a look at appveyor errors, because I'm starting to get out of ideas. I tried a lot of different things, including copying setup from rayon crate that works for them, but yields linker errors in our case.

@KodrAus

This comment has been minimized.

Copy link
Contributor

KodrAus commented Nov 10, 2017

@fuine Sure. I've got a Windows box handy so will do some troubleshooting over the weekend.

@fuine

This comment has been minimized.

Copy link
Contributor Author

fuine commented Nov 11, 2017

Cool, thanks. Just heads up: compile tests work on my machine using both Windows and Linux, they also work on travis, it's just that for some reason appveyor build refuses to find lazy_static crate even though it currently is implemented the same way rayon has it set up (or so I think)

@KodrAus

This comment has been minimized.

Copy link
Contributor

KodrAus commented Nov 12, 2017

So this is actually failing on my local environment too. Which is probably good news

@fuine

This comment has been minimized.

Copy link
Contributor Author

fuine commented Nov 12, 2017

That's great news, however could you test it as well on 60e11d8 ?

@KodrAus

This comment has been minimized.

Copy link
Contributor

KodrAus commented Nov 12, 2017

Yep, specifying the target directly when running the compile tests seems to cause it to fail to find the lazy_static crate. What if we remove the explicit --target calls from the script? We shouldn't actually need them since we only install the appropriate toolchain in each environment.

@fuine

This comment has been minimized.

Copy link
Contributor Author

fuine commented Nov 12, 2017

I don't have any indepth knowledge of appveyor's config, but if you don't specify target doesn't it fallback on w/e target it has set as default/finds first? Also if this is really a proble with --target flag then we should probably open an issue in cargo/rust repository

@fuine

This comment has been minimized.

Copy link
Contributor Author

fuine commented Nov 12, 2017

Actually at this point I'm willing to test that and just manually see the logs to check which targets are used if we don't specify them explicitly.

@KodrAus

This comment has been minimized.

Copy link
Contributor

KodrAus commented Nov 12, 2017

I don't have any indepth knowledge of appveyor's config, but if you don't specify target doesn't it fallback on w/e target it has set as default/finds first?

I think you're right, and since you're setting the default appropriately this should be fine.

My guess is that this is a problem with the rustflags we're giving compiletest, rather than a problem with compiletest itself. When cargo builds for a specific target, the results don't end up in the target/debug folder, they go into target/%TARGET%/debug, which isn't included in our script. It's probably easier to just exclude the target.

@fuine fuine force-pushed the fuine:compiletest branch from dc9f206 to e5b6815 Nov 12, 2017

@fuine

This comment has been minimized.

Copy link
Contributor Author

fuine commented Nov 12, 2017

Thanks for the help, I've squashed previous commits and added some notes on the tests. I think it's potentially mergeable once CI goes green, but I can also squash the last commit with the first one - I want to preserve 6529b07 because it explicitly fixes a regression that I introduced in my previous PR.

@KodrAus

This comment has been minimized.

Copy link
Contributor

KodrAus commented Nov 12, 2017

That history looks good to me 👍 I'm happy to merge this in once it's all green.

Thanks for doing this @fuine! Your efforts to improve a bunch of aspects of the library are really appreciated! 😃

@KodrAus KodrAus changed the title WIP Add support for compiler tests Add support for compiler tests Nov 12, 2017

@KodrAus KodrAus merged commit f012d9f into rust-lang-nursery:master Nov 12, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@fuine fuine deleted the fuine:compiletest branch Nov 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.