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

Compile on all travis platforms #553

Merged
merged 5 commits into from
Apr 7, 2017
Merged

Compile on all travis platforms #553

merged 5 commits into from
Apr 7, 2017

Conversation

berkowski
Copy link
Contributor

Addresses #534

One test failing:

failures:

---- sys::test_select::test_select stdout ----

	thread 'sys::test_select::test_select' panicked at 'assertion failed: `(left == right)` (left: `1`, right: `0`)', test/sys/test_select.rs:45

note: Run with `RUST_BACKTRACE=1` for a backtrace.

failures:

    sys::test_select::test_select

test result: FAILED. 57 passed; 1 failed; 0 ignored; 0 measured

error: test failed

This failure is consistent. I suspect it may stem from the nix implementation of fd_set, but nothing immediate jumps out as a cause.

@kamalmarhubi
Copy link
Member

hrm not sure why CI didn't run on this....

I am going to trust that the constants are correct, and hopefully some day we'll actually tackle #341. Could you add something to the changelog for this, then I'll merge it. Thanks!

@berkowski
Copy link
Contributor Author

I usually enable testing on all branches and then disable before finalizing the PR. I've re-enabled testing on all branches in .travis.yml.

What additions to the changelog were you thinking of? I'm making only the minimum number of changes to get things compiling. Switching over wholesale to libc defined constants is something that probably should be done but I'm trying to constrain the PR's to just what's needed at the moment. I'm not sure how that fits nicely into a changelog line :)

@berkowski berkowski changed the title Fix ppc compile errors Compile on all travis platforms Mar 10, 2017
@berkowski
Copy link
Contributor Author

Expanded the scope of the PR a bit to include additional changes I had lined up for #535

nix should now pass compile checks on all the current travis platforms. It still fails a number of runtime tests.

@kamalmarhubi I'm happy to put some notes in the changelog here, or would you rather wait until #536 is ready to go?

@berkowski
Copy link
Contributor Author

@kamalmarhubi Not sure if you still wanted some notes in the changelog for this or if you wanted to wait until #536 is ready.

@Susurrus
Copy link
Contributor

Susurrus commented Apr 6, 2017

@berkowski This does look pretty good to me, especially since it fixed the powerpc stuff that we've already had other people attempt. Two things:

  • Can you remove the commit with changes to .travis.yml?
  • Can you add a changelog entry that says something as simple as "Fixed compilation on Linux/PowerPC, etc." where you replace "etc." with all known platforms fixed by this?
  • Can you also fix the compilation warning on test/sys/test_aio.rs:185:1 where signaled should be upper-case? I'd like to lump that in with this PR since it's a build fix.

So we'll defer on fixing the tests to #536.

@Susurrus Susurrus mentioned this pull request Apr 6, 2017
@berkowski
Copy link
Contributor Author

I'll try to get to those changes this weekend.

@berkowski
Copy link
Contributor Author

Weekend comes early-ish.

Updated changelog, reverted .travis.yml, renamed the static variable in test_aio.rs

@Susurrus
Copy link
Contributor

Susurrus commented Apr 7, 2017

Does the signaled test work? You changed the declaration to all-caps, but not the usages. But since signaled isn't a constant, let's not worry about capitalizing it. Can you drop the last commit? I think this is good to push once you've done that.

@berkowski
Copy link
Contributor Author

Ugh.. getting sloppy. Sorry. Removed 151f9c8

@Susurrus Susurrus merged commit 67ae092 into nix-rust:new-ci/master Apr 7, 2017
@Susurrus
Copy link
Contributor

Susurrus commented Apr 7, 2017

Thanks, @berkowski! Now let's see how this shakes out on Travis and we can move on to #536.

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.

3 participants