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.

Sign up for GitHub

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

Jump to bottom

Mutate command args #87420

Closed
wants to merge 7 commits into from
Closed

Mutate command args #87420

wants to merge 7 commits into from

Conversation

cehteh
Copy link

@cehteh cehteh commented Jul 24, 2021

Finished implementation, the 'windows' bits are untested by me, i hope they pass the CI. Otherwise, awaiting comments.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 24, 2021
@Mark-Simulacrum
Copy link
Member

r? @joshtriplett - I feel like I saw some T-libs discussion of these APIs recently.

FWIW it'd be ideal to integrate more context into the PR description (presuming I'm recalling correctly that this context exists).

@cehteh
Copy link
Author

cehteh commented Aug 1, 2021

r? @joshtriplett - I feel like I saw some T-libs discussion of these APIs recently.

FWIW it'd be ideal to integrate more context into the PR description (presuming I'm recalling correctly that this context exists).

I didn't know under what circumstances github makes automatic links/references. Usually I made this explicit, but just forgotten to do on the PR request itself.

First discussion is on: https://internals.rust-lang.org/t/lack-of-api-mutating-args-at-std-command/14908
For the implementation I opened a issue: #87379

@joelpalmer joelpalmer added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 17, 2021
@inquisitivecrystal inquisitivecrystal added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Aug 24, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 13, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 28, 2021
@bors
Copy link
Contributor

bors commented Oct 1, 2021

☔ The latest upstream changes (presumably #89414) made this pull request unmergeable. Please resolve the merge conflicts.

@joshtriplett
Copy link
Member

I think I remember this being discussed elsewhere, but I don't remember the outcome:

  • I think arg_set(0, ...) should work on UNIX, and act equivalent to calling arg0, rather than asserting.
  • I don't think we should add the args_new helper; I think .args_clear().args(...) seems clearer.

@joshtriplett joshtriplett added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 4, 2021
@JohnCSimon
Copy link
Member

Ping from triage:
@cehteh can you please address the comments and set S-waiting-on-review when you're done? Thank you.

@cehteh
Copy link
Author

cehteh commented Oct 24, 2021

I think I remember this being discussed elsewhere, but I don't remember the outcome:

  • I think arg_set(0, ...) should work on UNIX, and act equivalent to calling arg0, rather than asserting.

As discussed in https://internals.rust-lang.org/t/lack-of-api-mutating-args-at-std-command/14908/6
I came to the conclusion it is better not to touch argv[0] here, because

  • none of the other .args*() API's can change it either
  • .clear() has to preserve it
  • it is 'slightly' special in semantics
  • changing it is not portable to all operating systems

I could imagine a .set_program_name() member function. This would be made more portable to OS'es which support this in other ways. Question is if it should go into this PR or being another ticket/PR since that would some more investigation/work how to make it portable and I haven't thought about what to do if such is not supported (rust emulation, conditional API, some Ext trait, runtime error result, ...).

  • I don't think we should add the args_new helper; I think .args_clear().args(...) seems clearer.

Ok I'll remove that later today. I wasn't sure if its a nice to have or unnecessary.

@cehteh
Copy link
Author

cehteh commented Oct 24, 2021

Ping from triage: @cehteh can you please address the comments and set S-waiting-on-review when you're done? Thank you.

how do i set S-waiting-on-review ?

@hkratz
Copy link
Contributor

hkratz commented Oct 24, 2021

Like this:
@rustbot label -S-waiting-on-author +S-waiting-on-review

(rustbot documentation)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 24, 2021
@bors
Copy link
Contributor

bors commented Oct 25, 2021

☔ The latest upstream changes (presumably #90042) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@cehteh
Copy link
Author

cehteh commented Oct 25, 2021

tidy error: /checkout/src/doc/rustc-dev-guide/ci/date-check/Cargo.toml doesn't have edition = "2021" on a separate line
tidy error: /checkout/src/doc/reference/style-check/Cargo.toml doesn't have edition = "2021" on a separate line

These errors exist in upstream I merged, not being introduced by this PR.

Will try to merge or rebase again when that becomes fixed.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Nov 11, 2021

☔ The latest upstream changes (presumably #90784) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 5, 2021
@cehteh
implement std::process::Command::args_clear()
a2fbdd3 
see rust-lang#87379

Only unix implementation yet.
@cehteh
implement std::process::Command::arg_set()
c420fdc 
Only unix implementation yet.
@cehteh
implement std::process::Command::args_new()
6a10a48
@cehteh
FIX: assert >= 1 in process::command::arg_set()
816e4ec 
We do not want to set argv[0] here. This needs to be asserted at any time.
Assertion for the index within range is only a debug_assert becasue its a mere
helpful error message, indexing later would assert on out of range access anyway.
@cehteh
Provide the windows implementation of args_clear() and arg_set()
ca62a97
@cehteh
DOCFIX: arg_set() lacked doc/panics
9d69f08
@cehteh
Revert "implement std::process::Command::args_new()"
45d1780 
This reverts commit 626d0e71b1e82c4804cec1a2bd03c3c53e4368ad.

* removes 'args_new()' as requested in rust-lang#87420 (comment)

* still disallow argv0 mutation as I explained in rust-lang#87420 (comment)
@cehteh
Copy link
Author

cehteh commented Dec 8, 2021

Had some f*ckup with submodule hashes slipped into the commit. Fixed now, ready to merge.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 6, 2022
@Dylan-DPC
Copy link
Member

@joshtriplett this is ready for review

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 20, 2022
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 24, 2022
@Dylan-DPC
Copy link
Member

r? @rust-lang/libs-api

@m-ou-se
Copy link
Member

m-ou-se commented May 16, 2022

I'm not convinced we should have this at all. Even the existing Command::get_args was quite controversial, since we don't want to provide a full Vec-like interface on Command. If you want to flexibly work with arguments before running the Command, it's probably better to just use a regular Vec before passing it to Command::args().

@rust-lang/libs-api - Does anyone else think we should have this?

@Dylan-DPC
Copy link
Member

closing this based on the last comment

@Dylan-DPC Dylan-DPC closed this Jun 2, 2022
@Dylan-DPC Dylan-DPC removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 2, 2022
@cehteh cehteh mentioned this pull request Feb 28, 2023
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@m-ou-se m-ou-se

Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet