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

Merge New CI Infrastructure Based on Trust/Cross Into Master #536

Merged
merged 28 commits into from
Apr 15, 2017

Conversation

posborne
Copy link
Member

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.

@Susurrus
Copy link
Contributor

Susurrus commented Apr 5, 2017

@berkowski @posborne I'd really like to get this merged and active for nix ASAP. What're the next steps for this work as I haven't been following it.

From my point of view, I think as long as this isn't a regression from the previous CI (as in the original platforms still pass) I'd like to get this merged. Once it's in-tree we can start to fix the other platforms as we go.

@berkowski
Copy link
Contributor

#553 seems to solve all the compile-time issues for the architectures covered by trust but it's been pending a final review for the last 3 weeks.

Once that gets merged then the whole new-ci/master branch will need to be updated to the current working master, fixing any issues along the way.

@Susurrus
Copy link
Contributor

Susurrus commented Apr 5, 2017

@berkowski Excellent, then I'll take a look at #553 either today or tomorrow and we'll get that merged soon then (I'm a maintainer now). And then we can take another crack at this.

@berkowski
Copy link
Contributor

Excellent (and congrats!)

@Susurrus
Copy link
Contributor

Susurrus commented Apr 7, 2017

One thing that I'd like to see added to this PR is an addition to the README describing which platforms nix officially supports as Tier 2 architectures (defined as only compiled-tested) and Tier 1 architectures (defined as compile- and run-tested). If all platforms have tests run on them, there's no point in distinguishing between Tier 1 and Tier 2, but let's still document the supported platforms. This will also help me when reviewing these changes so I know what should come out of this work.

os: # OSX included in build matrix explicitly
- linux
# default job
- TARGET=x86_64-unknown-linux-gnu
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs an env: after the - so that it actually sets the environment variable in a way that Travis understands.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I think this should be done within the matrix section. If you look at the Travis output, you see TARGET being declared twice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, found out this is a bug in Travis CI that they haven't put much effort into fixing: travis-ci/travis-ci#4681

@Susurrus
Copy link
Contributor

Susurrus commented Apr 7, 2017

I'd also like to understand the reasoning for not all Rust platforms supported by trust/cross being tested. There are some targets commented-out so I was wondering how those were decided.

@asomers
Copy link
Member

asomers commented Apr 8, 2017

@Susurrus FreeBSD and NetBSD aren't tested because Travis doesn't support those operating systems. TRUST builds for them by using a cross-compiler, but it can't cross-test.

@Susurrus
Copy link
Contributor

Susurrus commented Apr 8, 2017

@asomers Thanks, that makes sense. We should document that in the README by listing them as Tier 2 platforms and also possibly in the .travis.yml file, though I'm less sure about that one.

@posborne Are you still the lead on this or are you otherwise occupied for a while?

os: # OSX included in build matrix explicitly
- linux
# default job
- TARGET=x86_64-unknown-linux-gnu
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I think this should be done within the matrix section. If you look at the Travis output, you see TARGET being declared twice.

@Susurrus
Copy link
Contributor

Susurrus commented Apr 8, 2017

As a summary for everyone, we currently have the following outstanding issues with this PR:

  • Fix SIGABRT for i686-linux-musl
  • Fix SIGSEGV for x86_64-unknown-linux-musl
  • Fix Travis issue for i686-apple-darwin
  • Fix long-running sys::test_aio::test_lio_listio_signal test for mips-unknown-linux-gnu
  • Fix long-running sys::test_aio::test_lio_listio_signal test for mipsel-unknown-linux-gnu
  • Fix sys::test_select::test_select for powerpc-unknown-linux-gnu

And I just realized this, but instead of targeting current stable Rust, we should target 1.9, which is our minimally supported version. Maybe then instead of the two nightly platforms we have, we would test stable. Nightly can give spurious failures, but it's of value to the compiler team, so I'm not so sure about this one.

I'd propose that we only address the i686-apple-darwin and powerpc-unknown-linux-gnu target failures as they should be easy and I think we should support those platforms. I'd then propose we disable build running tests on the other platforms for now so that we're all-green and open tracking issues for adding Tier 1 support for those untested platforms.

Since this testing is already more comprehensive than the current testing we get a step forward and this will allow us to move forward with other PRs that are basically blocking on this more comprehensive testing. Thoughts anyone?

@asomers
Copy link
Member

asomers commented Apr 8, 2017

Aren't the sys::test_aio::test_lio_listio_signal failures fixed by posborne/rust-nix@0e3ef36 ? Do you have that change in your branch?

@Susurrus
Copy link
Contributor

Susurrus commented Apr 8, 2017

@asomers Maybe they did, I've cherry-picked that commit and fixed the TARGET value for x86_64-unknown-linux-gnu to move this along a bit, so we'll see what Travis says...

@Susurrus
Copy link
Contributor

Susurrus commented Apr 8, 2017

For posterity looks like that commit was being tracked in #539, whoops.

@berkowski
Copy link
Contributor

Remind me what the i686-apple-darwin failure is? Seems like it passed in the last round.

I'd also recommend merging/rebasing the travis branch off the current master head before too much work is done. A lot of other PR's have been merged into the main branch since the CI work started.

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
@asomers
Copy link
Member

asomers commented Apr 15, 2017

PR #579 might help with the mips failures.

Some entries were erroneously listed under the 0.8.0 release.
@Susurrus
Copy link
Contributor

Okay, everyone, I'm planning on merging this. I've segregated out failing targets into an allowed_failure section, got the CHANGELOG up to date, and updated the README with supported targets and the new minimum Rust version (1.13). I haven't heard any negative feedback on anything I've proposed so far so I'm going to assume that silence is a tacit agreement with this plan.

One thing that I should note is that I made it more explicit in the README about how well we support the various architectures. I split them into 3 tiers, in a similar vein to how Rust has 3 tiers of platform support.

@Susurrus
Copy link
Contributor

@homu r+

@berkowski
Copy link
Contributor

Nice work so far and good thought making it explicit in the readme what degrees of support are expected for each platform.

@Susurrus
Copy link
Contributor

Okay, let's see if this will work this time:

@homu r+

@Susurrus
Copy link
Contributor

@kamalmarhubi @posborne @fiveop Homu doesn't seem to be doing anything, but I'm not really familiar with Homu to debug this. Can any of you guys comment on why it's not working?

@kamalmarhubi
Copy link
Member

Weird. Let me try!

@homu r+

@homu
Copy link
Contributor

homu commented Apr 15, 2017

📌 Commit 56f5a0a has been approved by kamalmarhubi

@kamalmarhubi
Copy link
Member

Hrm. That's upsetting.

@homu homu merged commit 56f5a0a into master Apr 15, 2017
@homu
Copy link
Contributor

homu commented Apr 15, 2017

⚡ Test exempted - status

homu added a commit that referenced this pull request Apr 15, 2017
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.
@kamalmarhubi
Copy link
Member

@Susurrus I'm not sure what's up. As far as I know it should work. At the same time, Homu has a database error, and has done for a while: http://homu.io/l/

@kamalmarhubi
Copy link
Member

I will see if we can switch to bors-ng, which has a hosted version as well.

@Susurrus
Copy link
Contributor

@kamalmarhubi There's a synchronize button on the homu website and it asked for permissions to my account and nix-rust on GitHub. Do I need to do that so that I recognizes me or something?

@kamalmarhubi
Copy link
Member

@Susurrus I just opened #580 to figure this out. You could try that, it sounds somewhat reasonable!

@posborne
Copy link
Member Author

🎉 🎉 I've been off the radar for a little while helping land a big change (with a bunch of Rust) at work and have been doing a very poor job of maintaining. Glad to see this land. Thank you @asomers @Susurrus @kamalmarhubi and anyone else who contributed for getting this across the finish line!

@kamalmarhubi
Copy link
Member

@berkowski too! This is a heroic effort, thanks to all of you! All I did was tell @homu to merge it. :-)

@Susurrus Susurrus deleted the new-ci/master branch April 19, 2017 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants