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

flock: Use fcntl constants directly from libc crate on Unix targets #57121

Merged
merged 2 commits into from
Jan 6, 2019

Conversation

glaubitz
Copy link
Contributor

Since the values for the fcntl constants can vary from architecture
to architecture, it is better to use the values defined in the libc
crate instead of assigning literals in the flock code which would
make the assumption that all architectures use the same values.

Fixes #57007

@rust-highfive
Copy link
Collaborator

r? @varkor

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 25, 2018
@glaubitz
Copy link
Contributor Author

This also requires two small fixes in the libc crate: rust-lang/libc#1186

@nagisa
Copy link
Member

nagisa commented Dec 26, 2018

This also requires two small fixes in the libc crate: rust-lang/libc#1186

liblibc is a submodule in rust tree, so you will have to update the submodule after the referenced PR lands and before this can/should land.

@glaubitz
Copy link
Contributor Author

Thanks for the heads-up! I will open a PR for that, too.

@nagisa
Copy link
Member

nagisa commented Dec 26, 2018

Would be fine to just have it all be in the same PR (i.e. this PR), unless you’re in a hurry to land this regardless of whether the liblibc change langs.

@glaubitz
Copy link
Contributor Author

You mean this PR and the one for updating liblibc? And I'm not in a hurry, I'll prepare the PR whatever suits you best. Let's just wait for the libc update first.

@varkor
Copy link
Member

varkor commented Dec 26, 2018

And I'm not in a hurry, I'll prepare the PR whatever suits you best.

I think it would be most straightforward just to update the submodule in this PR, seeing as how they're directly related.

@glaubitz
Copy link
Contributor Author

I think it would be most straightforward just to update the submodule in this PR, seeing as how they're directly related.

And I'm just trying to find out how to do that properly. I have updated a submodule in a PR before.

Care to explain?

@nagisa
Copy link
Member

nagisa commented Dec 26, 2018

Oh, it seems that we have recently stopped using the submodule approach and are now pulling the dependency in via cargo from crates.io instead... Which essentially means that all you need to do is to update the relevant Cargo.toml files to use the new version from crates.io and it should work just as one would expect.

cc @alexcrichton liblibc probably needs a new release, right?

@glaubitz
Copy link
Contributor Author

Ah, right. I was already wondering why git submodule didn't list liblibc. That explains it :).

@glaubitz
Copy link
Contributor Author

@alexcrichton Oh, before you release. I think @karcherm has two fixes for sparc64 in libc ready.

@karcherm
Copy link
Contributor

One of the patches @glaubitz is talking about is [https://github.com/rust-lang/libc/pull/1187]. I'm unsure whether it should be merged or not. The second one is work in progress to enable reliable stack overflow detection on sparc64. It is not ready yet, but possibly in a couple of hours.

@glaubitz
Copy link
Contributor Author

@alexcrichton I would suggest to do a new libc now, so we can get this PR merged.

We can do another release once @karcherm has pushed his fixes.

@varkor varkor added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 1, 2019
bors added a commit to rust-lang/libc that referenced this pull request Jan 2, 2019
Bump version to 0.2.46

New release required for rust-lang/rust#57121
bors added a commit to rust-lang/libc that referenced this pull request Jan 3, 2019
Bump version to 0.2.46

New release required for rust-lang/rust#57121
bors added a commit to rust-lang/libc that referenced this pull request Jan 3, 2019
Bump version to 0.2.46

New release required for rust-lang/rust#57121
@glaubitz
Copy link
Contributor Author

glaubitz commented Jan 4, 2019

I'm trying to update the libc crate now for this PR but that fails with:

glaubitz@epyc:..rust/rust> cargo update -p libc
    Updating crates.io index
error: no matching package named `core` found                                                                                                                                                
location searched: registry `https://github.com/rust-lang/crates.io-index`
required by package `backtrace-sys v0.1.27`
    ... which is depended on by `backtrace v0.3.11`
    ... which is depended on by `error-chain v0.12.0`
    ... which is depended on by `cargo_metadata v0.6.2`
    ... which is depended on by `miri v0.1.0 (/local_scratch/glaubitz/rust/rust/src/tools/miri)`
glaubitz@epyc:..rust/rust>

I made a fresh checkout to make sure my repo isn't corrupted but that didn't help.

Anyone got an idea what's wrong?

@alexcrichton
Copy link
Member

Hm I think you may need to make sure to use the nightly cargo to update libc, but you can also just update one of the version requirements in this repository and then ./x.py build

@glaubitz
Copy link
Contributor Author

glaubitz commented Jan 4, 2019

@alexcrichton Updating cargo to nightly did the trick. I just updated libc and pushed a new revision.

Should be all set now :).

@bors
Copy link
Contributor

bors commented Jan 6, 2019

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

@glaubitz
Copy link
Contributor Author

glaubitz commented Jan 6, 2019

@alexcrichton @nagisa r?

@@ -16,6 +16,12 @@ cfg_if! {
use std::os::unix::prelude::*;
use libc;

pub const F_RDLCK: libc::c_short = libc::F_RDLCK as libc::c_short;
Copy link
Member

Choose a reason for hiding this comment

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

Why is it necessary to have these as constants here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'll push an updated version in a second.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed :).

Since the values for the fcntl constants can vary from architecture
to architecture, it is better to use the values defined in the libc
crate instead of assigning literals in the flock code which would
make the assumption that all architectures use the same values.

Fixes rust-lang#57007
@nagisa
Copy link
Member

nagisa commented Jan 6, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Jan 6, 2019

📌 Commit 1c4823e has been approved by nagisa

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 6, 2019
@bors
Copy link
Contributor

bors commented Jan 6, 2019

⌛ Testing commit 1c4823e with merge 6b2c311...

bors added a commit that referenced this pull request Jan 6, 2019
flock: Use fcntl constants directly from libc crate on Unix targets

Since the values for the fcntl constants can vary from architecture
to architecture, it is better to use the values defined in the libc
crate instead of assigning literals in the flock code which would
make the assumption that all architectures use the same values.

Fixes #57007
@bors
Copy link
Contributor

bors commented Jan 6, 2019

☀️ Test successful - status-appveyor, status-travis
Approved by: nagisa
Pushing 6b2c311 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants