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

deprecate before_exec in favor of unsafe pre_exec #58059

Open
wants to merge 8 commits into
base: master
from

Conversation

@RalfJung
Copy link
Member

RalfJung commented Feb 1, 2019

Fixes #39575

As per the lang team decision:

The language team agreed that before_exec should be unsafe, and leaves the details of a transition plan to the libs team.

Cc @alexcrichton @rust-lang/libs how would you like to proceed?

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Feb 1, 2019

r? @withoutboats

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

@alexcrichton alexcrichton added the T-libs label Feb 1, 2019

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 1, 2019

Thanks for this @RalfJung!

In terms of rollout I think we'll want to schedule this to be stabilized in the future when the replacement, pre_exec, is also on the stable channel. I think the rustc_deprecated tag, as written, will start warning immediately on nightly which provides a subpar experience for stable crates that test on all three versions. I'd recommend 1.37.0 because that means the replacement, pre_exec, has baked on stable for a full release.

In the documentation of before_exec, could it also be mentioned that the function, while stable and usable, is scheduled for a future deprecation. Additionally in the documentation for pre_exec in the safety section, I recall there being mentions on the issue about POSIX saying that it's only defined to do async-signal-safe things or something like that, so it may be worth adding language along those lines.

For other small pieces, could the formatting be updated where necessary to account for this change? There's a few argument lists out of sync and the tests aren't quite indented in an idiomatic way.

For the actual name itself and the plan, let's...

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Feb 1, 2019

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Feb 1, 2019

@RalfJung

This comment has been minimized.

Copy link
Member Author

RalfJung commented Feb 2, 2019

I think the rustc_deprecated tag, as written, will start warning immediately on nightly which provides a subpar experience for stable crates that test on all three versions. I'd recommend 1.37.0 because that means the replacement, pre_exec, has baked on stable for a full release.

So I should change rustc_deprecated to 1.37.0 but leave the stabilization of pre_exec the way it was? Done.

In the documentation of before_exec, could it also be mentioned that the function, while stable and usable, is scheduled for a future deprecation.

It already says that this is deprecated? I clarified the wording a bit, but I do not understand what you are asking here.

in the documentation for pre_exec in the safety section, I recall there being mentions on the issue about POSIX saying that it's only defined to do async-signal-safe things or something like that, so it may be worth adding language along those lines.

Done.

There's a few argument lists out of sync

I do not see it, what are you referring to?

the tests aren't quite indented in an idiomatic way.

I ran rustfmt.

@alexcrichton
Copy link
Member

alexcrichton left a comment

So I should change rustc_deprecated to 1.37.0 but leave the stabilization of pre_exec the way it was? Done.

That's at least what I'm led to believe! We can always tweak it if this ends up not being the case.

It already says that this is deprecated?

Er sorry what I mean is to label it as "schedule to be deprecated" as we're going to have a number of months before it's actually deprecated, and I figured it'd be good to head off any confused users asking why it says it's deprecated but it's not actually labled or warned as deprecated yet.

I ran rustfmt.

Thanks!

Show resolved Hide resolved src/libstd/sys/redox/ext/process.rs Outdated
Show resolved Hide resolved src/libstd/sys/unix/process/process_common.rs Outdated
@RalfJung

This comment has been minimized.

Copy link
Member Author

RalfJung commented Feb 3, 2019

Er sorry what I mean is to label it as "schedule to be deprecated" as we're going to have a number of months before it's actually deprecated, and I figured it'd be good to head off any confused users asking why it says it's deprecated but it's not actually labled or warned as deprecated yet.

AFAIK this will be labelled as deprecated in the docs even if the version is still in the future. That's the point, actually: so that people read the docs do not add new uses.

So, this shouldn't be confusing.

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Feb 5, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 5, 2019

Ok, r=me when FCP finishes!

@Centril Centril added the relnotes label Feb 11, 2019

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Feb 15, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 15, 2019

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 15, 2019

📌 Commit e023403 has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 15, 2019

⌛️ Testing commit e023403 with merge 0f46f6e...

bors added a commit that referenced this pull request Feb 15, 2019

Auto merge of #58059 - RalfJung:before_exec, r=alexcrichton
deprecate before_exec in favor of unsafe pre_exec

Fixes #39575

As per the [lang team decision](#39575 (comment)):

> The language team agreed that before_exec should be unsafe, and leaves the details of a transition plan to the libs team.

Cc @alexcrichton @rust-lang/libs how would you like to proceed?
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 15, 2019

💔 Test failed - checks-travis

@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

rust-highfive commented Feb 15, 2019

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.

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)

@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented Feb 15, 2019

@bors retry -- apparently we can't clone the repo anymore on macOS

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Feb 20, 2019

@bors rollup

Centril added a commit to Centril/rust that referenced this pull request Feb 20, 2019

Rollup merge of rust-lang#58059 - RalfJung:before_exec, r=alexcrichton
deprecate before_exec in favor of unsafe pre_exec

Fixes rust-lang#39575

As per the [lang team decision](rust-lang#39575 (comment)):

> The language team agreed that before_exec should be unsafe, and leaves the details of a transition plan to the libs team.

Cc @alexcrichton @rust-lang/libs how would you like to proceed?

bors added a commit that referenced this pull request Feb 20, 2019

Auto merge of #58594 - Centril:rollup, r=Centril
Rollup of 12 pull requests

Successful merges:

 - #55632 (Deny the `overflowing_literals` lint for all editions)
 - #57656 (Deprecate the unstable Vec::resize_default)
 - #57997 (Wrap write_bytes in a function. Move docs)
 - #58059 (deprecate before_exec in favor of unsafe pre_exec)
 - #58122 (RangeInclusive internal iteration performance improvement.)
 - #58198 (Suggest removing parentheses surrounding lifetimes)
 - #58353 (Check the Self-type of inherent associated constants)
 - #58476 (Remove `LazyTokenStream`.)
 - #58511 (Const to op simplification)
 - #58555 (Add a note about 2018e if someone uses `try {` in 2015e)
 - #58588 (remove a bit of dead code)
 - #58589 (cleanup macro after 2018 transition)

Failed merges:

r? @ghost
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment