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

Revert "Function to convert OpenOptions to c_int" #77090

Merged
merged 1 commit into from
Sep 23, 2020

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Sep 23, 2020

Reverts #76110. This broke Rust's stability guarantees.

Closes #77089.

r? @joshtriplett

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 23, 2020
@dtolnay
Copy link
Member

dtolnay commented Sep 23, 2020

@bors r+ p=10

@bors
Copy link
Contributor

bors commented Sep 23, 2020

📌 Commit 15f08d6 has been approved by dtolnay

@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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 23, 2020
@jyn514
Copy link
Member Author

jyn514 commented Sep 23, 2020

@dtolnay is there a way to tell whether a PR breaks stability without eyeballing it or noticing from a bug report after the fact? It would be great to have it as part of the test suite somehow.

@jyn514 jyn514 added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. A-stability Area: `#[stable]`, `#[unstable]` etc. labels Sep 23, 2020
@bors
Copy link
Contributor

bors commented Sep 23, 2020

⌛ Testing commit 15f08d6 with merge c80f5e8ae0e4e51ae34a373092c84cd5f9fbb038...

@bors
Copy link
Contributor

bors commented Sep 23, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 23, 2020
@jyn514
Copy link
Member Author

jyn514 commented Sep 23, 2020

It failed building LLVM, it might be spurious?

  running: "D:/a/rust/rust/citools/clang-rust/bin/clang-cl.exe" "-nologo" "-MT" "-O2" "-Brepro" "-m64" "/Zl" "-D__func__=__FUNCTION__" "-FoD:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage1-std\\x86_64-pc-windows-msvc\\release\\build\\compiler_builtins-2a3f11e60d5138ca\\out\\divsc3.o" "-c" "D:\\a\\rust\\rust\\src/llvm-project/compiler-rt\\lib/builtins\\divsc3.c"
  exit code: 0xc0000005

  --- stderr


  error occurred: Command "D:/a/rust/rust/citools/clang-rust/bin/clang-cl.exe" "-nologo" "-MT" "-O2" "-Brepro" "-m64" "/Zl" "-D__func__=__FUNCTION__" "-FoD:\\a\\rust\\rust\\build\\x86_64-pc-windows-msvc\\stage1-std\\x86_64-pc-windows-msvc\\release\\build\\compiler_builtins-2a3f11e60d5138ca\\out\\divsc3.o" "-c" "D:\\a\\rust\\rust\\src/llvm-project/compiler-rt\\lib/builtins\\divsc3.c" with args "clang-cl.exe" did not execute successfully (status code exit code: 0xc0000005).

@bors retry p=10

@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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 23, 2020
@bors
Copy link
Contributor

bors commented Sep 23, 2020

⌛ Testing commit 15f08d6 with merge a6008fa...

@bors
Copy link
Contributor

bors commented Sep 23, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: dtolnay
Pushing a6008fa to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 23, 2020
@bors bors merged commit a6008fa into master Sep 23, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 23, 2020
@jyn514 jyn514 deleted the revert-76110-convert-openoptions-cint branch September 23, 2020 16:12
@joshtriplett
Copy link
Member

Interesting; I'd agree with the assessment elsewhere that OpenOptionsExt should really have been sealed, but since it can't be, this is indeed a stability break. I don't think we can do much better than a default implementation that calls unimplemented!() with an appropriate message, here.

@jyn514
Copy link
Member Author

jyn514 commented Sep 24, 2020

Whatever happens, we should also add a test for the current behavior so this doesn't slip into nightly again.

@joshtriplett
Copy link
Member

joshtriplett commented Oct 5, 2020

It occurs to me that we could add another trait, this one sealed, to handle this, rather than extending the existing trait.

(It's unfortunate that we can't just have a platform-specific implementation of From.)

@dtolnay dtolnay assigned dtolnay and unassigned joshtriplett Mar 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-stability Area: `#[stable]`, `#[unstable]` etc. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression in nightly-2020-09-23: "missing as_flags in implementation" of OpenOptionsExt
6 participants