Skip to content
This repository has been archived by the owner on Apr 5, 2024. It is now read-only.

Fix exit code of rust-semverver in tests #93

Merged
merged 10 commits into from
Mar 1, 2019

Conversation

gnzlbg
Copy link
Contributor

@gnzlbg gnzlbg commented Feb 26, 2019

So it turned out that the bug with rust-semverver returning an incorrect exit code on error (it always returned success) wasn't in rust-semverver, but in rustc itself. This is now fixed upstream (rust-lang/rust#58400), which makes all tests that should return an error exit code fail. This PR fixes that.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Feb 26, 2019

@ibabushkin with this change, I think we can start using rust-semverver in production CI. I'd like to start by adding it to rust-lang/libc to verify that PRs do not make API breaking changes, since this is such a critical library for the ecosystem, and it is not easily possible to test it on all targets that Rust currently supports.

The only blocker is that rust-semverver would need to be "instantaneously" maintained. That is, if nightly breaks it, it would need to be fixed immediately, and a new version pushed, or otherwise CI of all projects using it would fail.

I don't know what the best way is to address that.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Feb 26, 2019

Ok so this fixes things for older libc versions, but is not enough to use newer libc versions, because newer versions have a build.rs script, and that does not appear to be supported.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Feb 26, 2019

but is not enough to use newer libc versions, because newer versions have a build.rs script, and that does not appear to be supported.

So the issue is that rust-semverver does not handle crates with multiple outputs at all. Thank you for leaving a TODO comment! It made really easy to track this down!

When build scripts are involved, this is what happens, at least for libc. There are three outputs with the libc name: the build of the build.rs itself into a build_program, the execution of this build_program, and then the build of the libc-xyz.rlib output.

So in my last commit I just hacked in the bare minimum logic to support this particular use case. I have no idea what other kinds of multiple outputs there are, and how to programatically find what we are interested in (just the final rlib). So I left the TODO in place since this is something that still should be solved at some point.


cc @ehuss - is there a way to extract from the build plan whether an output is a build script, a build program, an rlib, etc. ? If not, maybe adding a key to the json structure identifying these things might be worth doing.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Feb 26, 2019

So I have a PR to libc that uses this PR of rust-semverver on CI to verify that no API-breaking changes are introduced: rust-lang/libc#1154

@ehuss
Copy link

ehuss commented Feb 27, 2019

The building of a build script is noted as "target_kind": ["custom-build"], "compile_mode": "build"

The execution of the build script is noted as "compile_mode": "run-custom-build"

I'm not terribly familiar with semverver, but I'm guessing you only want lib targets? In that case, you want to filter for target_kind containing a lib kind. I don't know which kind of libs semverver supports, so you'll need to check on that. It can be "lib", "rlib", "dylib", "staticlib", "cdylib", etc.

IOW, you probably want to filter for target_kind being a valid lib kind that semverver supports.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Feb 27, 2019

Thank you @ehuss !

So I've updated the PR to check if the target_kind contains the lib sub-string for now. We can restrict it to certain kinds of libs if we encounter issues.

The building of a build script is noted as "target_kind": ["custom-build"], "compile_mode": "build"

The execution of the build script is noted as "compile_mode": "run-custom-build"

@ehuss This is the json output I get, and I don't see compile_mode in there: https://gist.github.com/gnzlbg/497c3b4b859ea49bc6154ff6c6abe3b3

We might be using a slightly older version of cargo. Is that a new field?

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Feb 27, 2019

So I've updated the libc PR with the latest changes here, and everything still works.

One thing we probably want to do in the near future is add support for validating APIs that depend on --features.

@ehuss
Copy link

ehuss commented Feb 28, 2019

We might be using a slightly older version of cargo. Is that a new field?

It was added about about 3 months ago in rust-lang/cargo#6331.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Feb 28, 2019

@ehuss ah that explains it, we are still using cargo = "0.32".


@ehuss there is some discussion here that might interest you: rust-dev-tools/dev-tools-team#45

@ibabushkin ibabushkin merged commit bd29cd6 into rust-lang:master Mar 1, 2019
@ibabushkin
Copy link
Contributor

Sorry this took so long, merged!

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Mar 1, 2019

Awesome, thank you! If you could do a release, I'll update the libc PR and libc will start using this in CI (allowing it to fail for the time being, so that if this temporarily breaks libc's build still passes).

@ibabushkin
Copy link
Contributor

ibabushkin commented Mar 1, 2019 via email

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants