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

Unify OsString/OsStr for byte-based implementations #58953

Merged
merged 1 commit into from Mar 22, 2019

Conversation

Projects
None yet
7 participants
@jethrogb
Copy link
Contributor

commented Mar 5, 2019

As requested in #57860

r? @joshtriplett

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Mar 5, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:00f55755:start=1551827454640017285,finish=1551827455546996046,duration=906978761
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
Setting environment variables from .travis.yml
---
[00:04:52]    Compiling panic_unwind v0.0.0 (/checkout/src/libpanic_unwind)
[00:04:59] error: use has missing stability attribute
[00:04:59]   --> src/libstd/sys/unix/ext/ffi.rs:37:1
[00:04:59]    |
[00:04:59] 37 | pub use crate::sys_common::ffi_bytes::*;
[00:04:59] 
travis_time:end:12e1d180:start=1551827466555934200,finish=1551827766510703537,duration=299954769337
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:03134afc

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@jethrogb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 5, 2019

[00:04:59] error: use has missing stability attribute
[00:04:59]   --> src/libstd/sys/unix/ext/ffi.rs:37:1
[00:04:59]    |
[00:04:59] 37 | pub use crate::sys_common::ffi_bytes::*;

Why is a stability attribute required now? There wasn't one on the pub mod before. What do I even put here? unstable(std_internals)?

@joshtriplett

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

This looks great!

I wish we could unify the doc comments as well, but even without that, this is a huge improvement.

@jethrogb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

I wish we could unify the doc comments as well, but even without that, this is a huge improvement.

I'll take suggestions on how to do that. As you can see, currently you need to use the respective modules, so they have to be different.

@joshtriplett

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

@jethrogb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

Ok cool. Do you have any comments on the stability attribute issue?

@joshtriplett

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

@jethrogb No, I don't know why that happens. I don't know why you'd need separate stability attributes here.

@rust-lang/libs?

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

If the compiler requires a stability attribute then one needs to be applied. The compiler will often ask for a stability attribute on something that isn't actually used in stability attribute processing, but theoretically can be taken into account one day

@jethrogb jethrogb force-pushed the jethrogb:jb/unify-ffi branch from 4005e4e to 223d881 Mar 7, 2019

@jethrogb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

Nevermind, I wasn't reading the error message correctly. Stability attributes are warranted here. Fixed now.

@jethrogb jethrogb force-pushed the jethrogb:jb/unify-ffi branch from 223d881 to 3bc4e76 Mar 7, 2019

@Dylan-DPC

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

ping from triage @joshtriplett waiting for your review on this

@joshtriplett

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 18, 2019

📌 Commit 3bc4e76 has been approved by joshtriplett

pub mod io;
pub mod mutex;
#[cfg(any(unix,

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Mar 20, 2019

Member

Could this #[cfg] annotation perhaps be removed to simply #![allow(dead_code)] in the module? It's not really critical we actually delete dead code here and maintaining this #[cfg] over time seems like it may get burdensome (especially if it's duplicated with the above)

This comment has been minimized.

Copy link
@jethrogb

jethrogb Mar 20, 2019

Author Contributor

This is not possible because the code doesn't compile as-is on Windows - see https://travis-ci.com/rust-lang/rust/jobs/186485408

I don't think maintaining the cfg (which I put back) is problematic, lots of sys/ has code like this.

Show resolved Hide resolved src/libstd/sys_common/mod.rs Outdated

@jethrogb jethrogb force-pushed the jethrogb:jb/unify-ffi branch from 3bc4e76 to fbc1aed Mar 20, 2019

@jethrogb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

Made requested changes

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

@bors: r=JoshTriplett

@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

📌 Commit fbc1aed has been approved by JoshTriplett

@jethrogb jethrogb force-pushed the jethrogb:jb/unify-ffi branch from fbc1aed to 7b1fc49 Mar 20, 2019

@lzutao

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

Force-pushed. This PR needs to be approved again.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

📌 Commit 7b1fc49 has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

⌛️ Testing commit 7b1fc49 with merge 987e145...

bors added a commit that referenced this pull request Mar 21, 2019

Auto merge of #58953 - jethrogb:jb/unify-ffi, r=alexcrichton
Unify OsString/OsStr for byte-based implementations

As requested in #57860

r? @joshtriplett
@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

💔 Test failed - status-appveyor

@jethrogb jethrogb force-pushed the jethrogb:jb/unify-ffi branch from 7b1fc49 to 2079df1 Mar 21, 2019

@jethrogb

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

Fixed rustdoc. It's a pretty ugly hack because rustdoc can document code that doesn't otherwise compile. But this is how it used to work, so I'm not too keen on changing that now.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

📌 Commit 2079df1 has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

⌛️ Testing commit 2079df1 with merge cb2f34d...

bors added a commit that referenced this pull request Mar 22, 2019

Auto merge of #58953 - jethrogb:jb/unify-ffi, r=alexcrichton
Unify OsString/OsStr for byte-based implementations

As requested in #57860

r? @joshtriplett
@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

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

@bors bors added the merged-by-bors label Mar 22, 2019

@bors bors merged commit 2079df1 into rust-lang:master Mar 22, 2019

1 check passed

homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.