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

Adjust for newer glibc on sparc64 #1187

Closed
wants to merge 1 commit into from

Conversation

karcherm
Copy link

This commit removes special cases for sparc64 which caused test failures on a sparc64
system (Debian, glibc 2.28). With these changes, the ABI tests pass except for the
known test failure from #1080

This commit removes special cases for sparc64 which caused test failures on a sparc64
system (Debian, glibc 2.28). With these changes, the ABI tests pass except for the
known test failure from rust-lang#1080
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @gnzlbg (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Dec 26, 2018

📌 Commit 86e6311 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Dec 26, 2018

⌛ Testing commit 86e6311 with merge 6a6d1af...

bors added a commit that referenced this pull request Dec 26, 2018
Adjust for newer glibc on sparc64

This commit removes special cases for sparc64 which caused test failures on a sparc64
system (Debian, glibc 2.28). With these changes, the ABI tests pass except for the
known test failure from #1080
@bors
Copy link
Contributor

bors commented Dec 26, 2018

💔 Test failed - status-travis

@karcherm
Copy link
Author

The CI failure is most likely due to an older libc version (Debian libc6-dev-sparc64-cross 0.24.x) compared to the host I created this patch for (Debian libc6-dev 0.28-3). How do we decide the target libc version for the libc crate?
As long as the reference versions on sparc64 stay unchanged, this PR is unneccessary. I made it because cargo test failed on the sparc64 system I use to create a different PR that is not yet ready.

@glaubitz
Copy link
Contributor

Do we need this commit to fix any failures on Debian sparc64 unstable with glibc 2.28?

@karcherm
Copy link
Author

Yes, as I said, it fixes all test failures (compiler warning that purposedfully gets escalated to an error for a member of utmpx and several outdated constants for netfilter stuff) that currently occur on Debian sparc64 unstable, except for one netfilter-related test failure that already has its own issue: #1080.

@karcherm
Copy link
Author

Well, this is about libc ABI tests, not about rustc tests. AFAIK libc ABI tests are not part of ./x.py test. It does not fix any of them.

@karcherm karcherm closed this Dec 27, 2018
@karcherm karcherm reopened this Dec 27, 2018
@gnzlbg
Copy link
Contributor

gnzlbg commented Dec 28, 2018

How do we decide the target libc version for the libc crate?

Historically we have chosen it "as old as possible" to be as compatible with as many libc versions as possible, but we have slowly moved to "as new as possible" because some things like consts (C enums, macro values, etc.) do change across libc versions in some OSes often.

If you need a newer libc version here, just update it as part of this PR.

@alexcrichton
Copy link
Member

Agreed with @gnzlbg we can just update libc in this repo's CI (which is installed here) and that should do the trick!

@gnzlbg
Copy link
Contributor

gnzlbg commented Jan 21, 2019

Any progress on this?

@glaubitz
Copy link
Contributor

@gnzlbg I'll take care of this. I just recently built fresh installation images for Debian sparc64. I'm actually the one who is responsible for the installation images in Debian Ports :). We can update the ones for powerpc and ppc64 as well, if necessary.

@bors
Copy link
Contributor

bors commented Feb 5, 2019

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

@gnzlbg
Copy link
Contributor

gnzlbg commented May 16, 2019

@karcherm could you check if these issues persist with libc master branch ? #1337 updated the glibc used in the sparc64 build jobs.

@glaubitz
Copy link
Contributor

@gnzlbg I think what needs to be done first is switching the CI to use a newer Debian sparc64 image from https://cdimage.debian.org/cdimage/ports/. I'm the person at Debian responsible for creating these images, so it's probably best when I send the PR for that to avoid pointing to links that might get moved.

@gnzlbg
Copy link
Contributor

gnzlbg commented May 16, 2019

@glaubitz I did that as part of #1337 (IIRC I updated to the latest image available there).

@glaubitz
Copy link
Contributor

@gnzlbg Okay, the 10.0 image you are using is reasonably new enough, I will overwrite it at some point with a new 10.0 one when Debian Buster is officially released.

Then @karcherm should be able to rebase and push his changes which require the CI to use the newer image.

@gnzlbg
Copy link
Contributor

gnzlbg commented May 16, 2019

Okay, the 10.0 image you are using is reasonably new enough, I will overwrite it at some point with a new 10.0 one when Debian Buster is officially released.

That would be awesome.

@bors
Copy link
Contributor

bors commented May 25, 2019

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

  1. git checkout update-sparc64 (switch to your branch)
  2. git fetch upstream master (retrieve the latest master)
  3. git rebase upstream/master -p (rebase on top of it)
  4. Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
  5. git push self update-sparc64 --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message
Auto-merging src/unix/notbsd/linux/other/mod.rs
CONFLICT (content): Merge conflict in src/unix/notbsd/linux/other/mod.rs
Automatic merge failed; fix conflicts and then commit the result.

@gnzlbg
Copy link
Contributor

gnzlbg commented May 29, 2019

This needs rebasing

@glaubitz
Copy link
Contributor

I'll take care of that.

@glaubitz
Copy link
Contributor

I just had a look at the patch and it looks like most of @karcherm 's changes are already in.

The only thing where the current code disagrees with @karcherm 's code is NFT_MSG_MAX which @karcherm has explicitly set to 22 while the current code uses 25.

@gnzlbg
Copy link
Contributor

gnzlbg commented May 31, 2019

The 25 is verified against the debian version used in CI, which is technically the only one that we fully-support. It should be forward compatible with newer kernel versions, but not with older versions.

@gnzlbg gnzlbg closed this May 31, 2019
@gnzlbg
Copy link
Contributor

gnzlbg commented May 31, 2019

FYI I'm closing this PR, it appears that most changes are in. Feel free to reopen or submit a new PR for newer changes.

@glaubitz
Copy link
Contributor

The 25 is verified against the debian version used in CI, which is technically the only one that we fully-support. It should be forward compatible with newer kernel versions, but not with older versions.

Okay, makes sense then. Thanks for having taken care of this.

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