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

Remove obsolete --disable-elf-tls configure switch. #41687

Merged
merged 1 commit into from
May 4, 2017

Conversation

rillian
Copy link
Contributor

@rillian rillian commented May 1, 2017

Support for disabling ELF-style thread local storage in
the standard library at configure time was removed in
pulls #30417 and #30678, in favour of a member in
the TargetOptions database. The new mentod respects
MACOSX_DEPLOYMENT_TARGET on macOS, addressing the
original use case for this configure optionl

However, those commits left the configure option itself
in place. It's no longer referenced anywhere and can
be removed.

@rust-highfive
Copy link
Collaborator

r? @aturon

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

@rillian
Copy link
Contributor Author

rillian commented May 1, 2017

Per #32047, this switch isn't connected to anything any more. In #30678 (comment) @alexcrichton asked that the switch remain available to avoid breaking anyone's build scripts. That was over a year ago though, and in particular I know we no longer need this for Gecko, so I think it's less confusing to remove it at this point.

@arielb1 arielb1 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 2, 2017
Support for disabling ELF-style thread local storage in
the standard library at configure time was removed in
pulls rust-lang#30417 and rust-lang#30678, in favour of a member in
the TargetOptions database. The new method respects
MACOSX_DEPLOYMENT_TARGET on macOS, addressing the
original use case for this configure option.

However, those commits left the configure option itself
in place. It's no longer referenced anywhere and can
be removed.
@aturon
Copy link
Member

aturon commented May 4, 2017

Note that @alexcrichton, in that comment, also suggested a deprecation warning (which is our usual practice before removal). AFAIK no such warning was implemented?

This is largely a policy question, so I'd like to bring in @rust-lang/compiler for their thoughts as well. (I don't personally have a strong opinion here.)

@alexcrichton
Copy link
Member

I doubt anyone ever really used this much less even knew about it, I don't think we need much ceremony to remove it personally, it's a tiny option when building rust that's easy to remove if it even needs to be.

@aturon
Copy link
Member

aturon commented May 4, 2017

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented May 4, 2017

📌 Commit 19cab63 has been approved by aturon

@bors
Copy link
Contributor

bors commented May 4, 2017

⌛ Testing commit 19cab63 with merge 222971f...

bors added a commit that referenced this pull request May 4, 2017
Remove obsolete --disable-elf-tls configure switch.

Support for disabling ELF-style thread local storage in
the standard library at configure time was removed in
pulls #30417 and #30678, in favour of a member in
the TargetOptions database. The new mentod respects
MACOSX_DEPLOYMENT_TARGET on macOS, addressing the
original use case for this configure optionl

However, those commits left the configure option itself
in place. It's no longer referenced anywhere and can
be removed.
@bors
Copy link
Contributor

bors commented May 4, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: aturon
Pushing 222971f to master...

@bors bors merged commit 19cab63 into rust-lang:master May 4, 2017
@rillian
Copy link
Contributor Author

rillian commented May 4, 2017

Thanks @aturon, @alexcrichton. I didn't implement a deprecation warning because the functionality was already broken. Anyone who was setting this in a build script wasn't getting what they expected anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants