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.

Sign up for GitHub

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

Jump to bottom

Add more fcntl and seal constants for Android/Linux #720

Merged
merged 8 commits into from
Aug 25, 2017
Merged

Add more fcntl and seal constants for Android/Linux #720

merged 8 commits into from
Aug 25, 2017

Conversation

Susurrus
Copy link
Contributor

No description provided.

@Susurrus
Copy link
Contributor Author

@alexcrichton I wanted to ask for your help here. The linux/fcntl.h and fcntl.h headers cannot be included simultaneously, so to test them I generated an additional binary just to test linux/fcntl.h includes. The file generation is working, but I'm not certain what the best way to hook things up in CI is. I think the best solution here is to generate this test executable for all platforms only including the appropriate constants on the correct systems and then have CI run both the main executable libc-test and also linux_fcntl on all platforms. This should just take some modifications to ci/run.sh if I'm understanding that correctly, is that what I should do? I want to make sure this is extensible because I plan on doing something similar to resolve #711.

@bors
Copy link
Contributor

bors commented Aug 11, 2017

☔ The latest upstream changes (presumably #722) made this pull request unmergeable. Please resolve the merge conflicts.

@Susurrus Susurrus force-pushed the fcntl branch 8 times, most recently from 1807196 to 05f1043 Compare August 11, 2017 19:25
@alexcrichton
Copy link
Member

Yeah this seems pretty reasonable to me! To suggest a way to simplify this, though, perhaps:

  • Instead of binaries, these could be tests/*.rs-style tests that are listed as harness = false
  • We could use Cargo's "test runner" support to specify how to execute binaries

I think if we do that it means that cargo test in the libc-test folder should "just work"?

@Susurrus Susurrus force-pushed the fcntl branch 2 times, most recently from 8806369 to 345364a Compare August 11, 2017 21:36
@Susurrus
Copy link
Contributor Author

@alexcrichton I implemented that and it's definitely a lot nicer. This also sets us up nicely for running additional specific tests (like for #711) for all platforms.

There are still build failures on OpenBSD, but that's a really complicated setup. Any way I could get some help debugging it? Comparing a good run and my recent failed run didn't reveal much. I think OPT_LEVEL being undefined may be part of the problem, but I don't see that anywhere in the project.

alexcrichton
alexcrichton reviewed Aug 12, 2017
View reviewed changes
ci/run.sh Outdated
cargo rustc --manifest-path libc-test/Cargo.toml --target $TARGET \
--test main -- -C link-args=-mios-simulator-version-min=7.0
cargo rustc --manifest-path libc-test/Cargo.toml --target $TARGET \
--test linux-fcntl -- -C link-args=-mios-simulator-version-min=7.0
Copy link
Member

Choose a reason for hiding this comment

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

It may be worth testing out RUSTFLAGS nowadays. I think at the time this code was written RUSTFLAGS didn't exist but nowadays I think it'd suffice for this, allowing us to basically just use cargo build --tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've fixed this on the latest build, but it failed on i386-ios because there were two main-* binaries. Which is weird, but maybe it's a caching issue. I'm not too certain what the caching policy is when set to cargo on Travis, but maybe that'll need to be turned off? Any other ideas here?

@alexcrichton
Copy link
Member

Ah drat I thought we removed some of these pieces awhile ago...

The OpenBSD testing is way crazier than everything else because it generates tests on the host and compiles them on the target (in the emulator). I think we should just jettison all that though, it's crazy complicated and not really worth it :(

@semarie
Copy link
Contributor

semarie commented Aug 12, 2017

the OPT_LEVEL problem isn't just that cargo in the image is out-of-date ? the used image is OpenBSD 6.0 so cargo should be 0.10.0.

I could try to update the image to 6.1 (latest stable). It will be cargo 0.16.0 and rustc 1.16.0.

@alexcrichton feel free to ping me for OpenBSD related issues.

@alexcrichton
Copy link
Member

@semarie my worry isn't necessarily exactly what's happening here but rather that the mechanism of testing OpenBSD is pretty crazy in this repo, and I think it's not pulling its weight any more. All other platforms cross-compile from linux and then we figure out how to emulate the binary, but we can't cross-compile to OpenBSD which makes it too hard to get a program running in there.

@Susurrus
Copy link
Contributor Author

Been a few days on this, do we know what direction we want to go with this? I'm willing to help push this forwards, but it looks like we are at an impasse in terms of direction here.

@alexcrichton
Copy link
Member

I think let's just remove OpenBSD support for now, and we can continue to shim how the other platforms are tested.

@bors
Copy link
Contributor

bors commented Aug 18, 2017

☔ The latest upstream changes (presumably #735) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member

I've actually gone ahead and removed OpenBSD from CI as we needed to do it anyway to get CI working again

@Susurrus
Add more fcntl and seal constants for Android/Linux
2e11d9e 
We now create an additional binary `linux_fcntl` for testing this
since there are header conflicts when including all necessary headers.
This binary is run on all platforms even though it's empty on all non-
Android/non-Linux platforms.

Testing has been switched from a custom binary to using a runner-less
test (or pair of tests). This means that for local development a simple
`cd libc-test && cargo test` will run all the tests. CI has also been
updated here to reflect that.
@Susurrus
Copy link
Contributor Author

Sounds good. Hopefully OpenBSD gets some official rustup/cargo support (whatever it is that's missing) so we can get better CI support for it.

Anyways, I updated this PR and it passed all tests.

@Susurrus Susurrus mentioned this pull request Aug 21, 2017
Merged
@bors
Copy link
Contributor

bors commented Aug 22, 2017

📌 Commit b7902df has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Aug 22, 2017

⌛ Testing commit b7902df with merge f9077b8...

bors added a commit that referenced this pull request Aug 22, 2017
@bors
Auto merge of #720 - Susurrus:fcntl, r=alexcrichton
f9077b8 
Add more fcntl and seal constants for Android/Linux
@bors
Copy link
Contributor

bors commented Aug 23, 2017

💥 Test timed out

@alexcrichton
Copy link
Member

@bors: retry

@Susurrus
Copy link
Contributor Author

It doesn't seem like bors is doing anything as Travis doesn't have any active builds. This applies to #737 as well.

@bors
Copy link
Contributor

bors commented Aug 23, 2017

⌛ Testing commit b7902df with merge be1c584...

bors added a commit that referenced this pull request Aug 23, 2017
@bors
Auto merge of #720 - Susurrus:fcntl, r=alexcrichton
be1c584 
Add more fcntl and seal constants for Android/Linux
@bors
Copy link
Contributor

bors commented Aug 23, 2017

💥 Test timed out

@alexcrichton
Copy link
Member

@bors: retry

@bors
Copy link
Contributor

bors commented Aug 23, 2017

⌛ Testing commit b7902df with merge 0faac5b...

bors added a commit that referenced this pull request Aug 23, 2017
@bors
Auto merge of #720 - Susurrus:fcntl, r=alexcrichton
0faac5b 
Add more fcntl and seal constants for Android/Linux
@bors
Copy link
Contributor

bors commented Aug 23, 2017

💔 Test failed - status-travis

@Susurrus
Copy link
Contributor Author

Failed due to timeout, but at least Travis has recently fixed the issues with the Mac builds, but there still is a massive backlog.

@alexcrichton
Copy link
Member

alexcrichton commented Aug 23, 2017 via email

@bors
Copy link
Contributor

bors commented Aug 23, 2017

⌛ Testing commit b7902df with merge 3e81e5e...

bors added a commit that referenced this pull request Aug 23, 2017
@bors
Auto merge of #720 - Susurrus:fcntl, r=alexcrichton
3e81e5e 
Add more fcntl and seal constants for Android/Linux
@Susurrus
Copy link
Contributor Author

This failed again on Travis with spurious errors on the Mac builders. Travis's backlog is getting down to a reasonable number, so trying this again will likely be successful.

@bors
Copy link
Contributor

bors commented Aug 24, 2017

💥 Test timed out

@alexcrichton
Copy link
Member

alexcrichton commented Aug 24, 2017 via email

@bors
Copy link
Contributor

bors commented Aug 24, 2017

⌛ Testing commit b7902df with merge e717ea6...

bors added a commit that referenced this pull request Aug 24, 2017
@bors
Auto merge of #720 - Susurrus:fcntl, r=alexcrichton
e717ea6 
Add more fcntl and seal constants for Android/Linux
@bors
Copy link
Contributor

bors commented Aug 25, 2017

💥 Test timed out

@alexcrichton
Copy link
Member

alexcrichton commented Aug 25, 2017 via email

@bors
Copy link
Contributor

bors commented Aug 25, 2017

⌛ Testing commit b7902df with merge 2a322c7...

bors added a commit that referenced this pull request Aug 25, 2017
@bors
Auto merge of #720 - Susurrus:fcntl, r=alexcrichton
2a322c7 
Add more fcntl and seal constants for Android/Linux
@bors
Copy link
Contributor

bors commented Aug 25, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 2a322c7 to master...

@bors bors merged commit b7902df into rust-lang:master Aug 25, 2017
@Susurrus
Copy link
Contributor Author

Omg it finally happened! Travis really needs to up their build slaves for Mac.

Anyways, thanks for staying on this @alexcrichton!

@Susurrus Susurrus deleted the fcntl branch August 25, 2017 01:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexcrichton alexcrichton alexcrichton left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Susurrus @bors @alexcrichton @semarie