-
Notifications
You must be signed in to change notification settings - Fork 682
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
Switch to trust-based CI #528
Conversation
ci/script.sh
Outdated
@@ -7,7 +7,7 @@ main() { | |||
cross build --target $TARGET | |||
cross build --target $TARGET --release | |||
|
|||
if [ -n $DISABLE_TESTS ]; then | |||
if [ ! -z $DISABLE_TESTS ]; then |
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.
Isn't this logic identical?
EDIT: I think what is really needed here may be quotes.
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.
Quotes would fix it. -n
is used in v0.1.1 of trust
while the -z
version is used on the HEAD of the master branch. I went with -z
figuring it'll be switched to that anyway at some point.
ci/script.sh
Outdated
cross run --target $TARGET --release | ||
# nix is a library -- no run target | ||
# cross run --target $TARGET | ||
# cross run --target $TARGET --release |
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.
Probably reasonable to update this commit to just delete these.
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.
Fair enough, I'll prune it out.
Ok, the errors that we are getting now seem legitimate. In order to distribute the load of fixing up the errors, I propose the following:
Sound reasonable? Thanks @berkowski for doing this work and thanks @japaric for putting together trust. It looks like the integration so far has been very reasonable. |
Down the road, it would be nice to test two versions at a minimum:
|
Ok, I changed the base in preparation for doing ^^^. For now, I think I would be OK with merging as long as there are no other changes you want to make in the first pass. From there, I think we create/assign issues for the individual platform problems and merge those in as needed (focus on either specific issues are people volunteer to get a specific platform working) and merge them into Thoughts @nix-rust/nix-maintainers @kamalmarhubi @fiveop ? |
Yes, please! This will make working on this project so much more enjoyable (for me :)). |
Add ppoll() This will currently fail CI, as the necessary changes haven't hit libc yet. That is tracked in rust-lang/libc#537. This does build on my computer using the changes tracked on that PR, so I'd appreciate any visual review of this code as it should be "done". I also wanted to get this submitted so hopefully it'd be in the queue for the 0.8 release.
Added: - arm-unknown-linux-gnueabi - arm-unknown-linux-musleabi Removed: - powerpc64-unknown-linux-gnu (not suppported by nix) - mips64el-unknown-linux-gnu (not suppported by nix) - mipsel-unknown-linux-gnu (not suppported by nix)
Removed: - mips64-unknown-linux-gnu - mips64el-unknown-linux-gnu - arm-unknown-linux-musleabi
Once that API key gets changed over to |
Sure, I'll go ahead and get this merged into the CI branch. I can update the API key using @nix-rust-bot which is what I have done in the past for docs and the like. |
NOTE: Manual merge to CI branch performed as not all tests are passing yet (as expected). |
Merge New CI Infrastructure Based on Trust/Cross Into Master See discussion on #528. This PR is to track the final decision of whether we are ready to merge the new CI infrastructure (and related bug fixes) back into master. Initially, we know that the branch is not ready to merge (as there are still failing tests). This PR acts as a single point of reference to get the current status of how close we are to merging things back into master. A number of issues with "TRUST CI" in the title have been created for the work that appears to be required to get us to the point where this branch can be merged.
An attempt to switch to using trust for multi-arch CI
I selected triplets based on CI coverage in
libc
andnix
:cargo test
not run)cargo test
not run)cargo test
not run)Notable issues:
cargo test
trust
target.travis.yml
will need to be updated with a github token fromnix-rust/nix
instead ofberkowski/nix
Other issues:
That being said,
trust
is catching a number of compile errors on various platforms. See https://travis-ci.org/berkowski/nix