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 PR 64324: dylibs export generics again (for now) #66018

Merged
merged 2 commits into from
Nov 1, 2019

Conversation

pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Nov 1, 2019

As discussed on PR #65781, this is a targeted attempt to undo the main semantic change from PR #64324, by putting dylib back in the set of crate types that export generic symbols.

The main reason to do this is that PR #64324 had unanticipated side-effects that caused bugs like #64872, and in the opinion of @alexcrichton and myself, the impact of #64872 is worse than #64319.

In other words, it is better for us, in the short term, to reopen #64319 as currently unfixed for now than to introduce new bugs like #64872.

Fix #64872

Reopen #64319

…rics export).

Includes the anticipated fallout to run-make-fulldeps test suite from
this change. (We need to reopen issue 64319 as part of landing this.)
(Many thanks to alex for 1. making this even smaller than what I had
originally minimized, and 2. pointing out that there is precedent for
having ui tests with crate dependency chains of length > 2, thus
allowing me to avoid encoding this as a run-make test.)
@rust-highfive
Copy link
Collaborator

r? @davidtwco

(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 Nov 1, 2019
@pnkfelix
Copy link
Member Author

pnkfelix commented Nov 1, 2019

r? @alexcrichton

@pnkfelix pnkfelix added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 1, 2019
@pnkfelix pnkfelix added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 1, 2019
@alexcrichton
Copy link
Member

@bors: r+

Thanks for all this @pnkfelix! And yeah the original PR (or so I thought) didn't impact share-generics in the general case, which I think you ended up finding out. I could be wrong though, and it could be good to revert anyway!

@bors
Copy link
Contributor

bors commented Nov 1, 2019

📌 Commit 6457914 has been approved by alexcrichton

@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 Nov 1, 2019
@pnkfelix
Copy link
Member Author

pnkfelix commented Nov 1, 2019

briefly discussed at today's T-compiler design meeting. Approved for beta-backport, (assuming it passes bors and lands successfully in nightly).

@pnkfelix pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Nov 1, 2019
tmandry added a commit to tmandry/rust that referenced this pull request Nov 1, 2019
…r=alexcrichton

Revert PR 64324: dylibs export generics again (for now)

As discussed on PR rust-lang#65781, this is a targeted attempt to undo the main semantic change from PR rust-lang#64324, by putting `dylib` back in the set of crate types that export generic symbols.

The main reason to do this is that PR rust-lang#64324 had unanticipated side-effects that caused bugs like rust-lang#64872, and in the opinion of @alexcrichton and myself, the impact of rust-lang#64872 is worse than rust-lang#64319.

In other words, it is better for us, in the short term, to reopen rust-lang#64319 as currently unfixed for now than to introduce new bugs like rust-lang#64872.

Fix rust-lang#64872

Reopen rust-lang#64319
bors added a commit that referenced this pull request Nov 1, 2019
Rollup of 16 pull requests

Successful merges:

 - #65112 (Add lint and tests for unnecessary parens around types)
 - #65470 (Don't hide ICEs from previous incremental compiles)
 - #65471 (Add long error explanation for E0578)
 - #65857 (rustdoc: Resolve module-level doc references more locally)
 - #65902 (Make ItemContext available for better diagnositcs)
 - #65914 (Use structured suggestion for unnecessary bounds in type aliases)
 - #65946 (Make `promote_consts` emit the errors when required promotion fails)
 - #65960 (doc: reword iter module example and mention other methods)
 - #65963 (update submodules to rust-lang)
 - #65972 (Fix libunwind build: Define __LITTLE_ENDIAN__ for LE targets)
 - #65977 (Fix incorrect diagnostics for expected type in E0271 with an associated type)
 - #65995 (Add error code E0743 for "C-variadic has been used on a non-foreign function")
 - #65997 (Fix outdated rustdoc of Once::init_locking function)
 - #66002 (Stabilize float_to_from_bytes feature)
 - #66005 (vxWorks: remove code related unix socket)
 - #66018 (Revert PR 64324: dylibs export generics again (for now))

Failed merges:

r? @ghost
@bors bors merged commit 6457914 into rust-lang:master Nov 1, 2019
@Mark-Simulacrum
Copy link
Member

It seems like the PR this reverts (#64324) did not ever land into beta, so presumably this PR also does not need to land into beta. De-nominating.

@Mark-Simulacrum Mark-Simulacrum removed beta-accepted Accepted for backporting to the compiler in the beta channel. beta-nominated Nominated for backporting to the compiler in the beta channel. labels Nov 1, 2019
@bjorn3
Copy link
Member

bjorn3 commented Nov 1, 2019

Thanks!

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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

linking of libtest failed
7 participants