unix: Relax escaping in `Debug` impl on `Command` #132484

madsmtm · 2024-11-02T02:53:42Z

The Debug output of Command is very useful for showing to the user the command that was executed when something went wrong. This is done for example by rustc when invoking an external tool like the linker fails.

It is also overly verbose, since everything is quoted, which makes it harder to read. Instead, we now first check if we're reasonably sure that an argument is simple enough that using it in the shell wouldn't need quoting, and then output it without quotes if possible.

An example of output before could look like this:

PATH="/a:/b" "cc" "foo.o" "-target" "arm64-apple-darwin11.0" "some path with spaces"

Now it looks like this:

PATH=/a:/b cc foo.o -target arm64-apple-darwin11.0 "some path with spaces"

This is naturally a behavioural change, but I believe it's allowed by the docs.

r? libs

library/std/src/sys/pal/unix/process/process_common.rs

library/std/src/sys/pal/unix/process/process_common/tests.rs

rustbot · 2024-11-04T01:09:54Z

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

library/std/src/process/tests.rs

thomcc · 2024-11-12T22:04:50Z

Seems fine but needs approval from libs-api I think (it's not an API change, but it is a behavioral one).

the8472 · 2024-11-12T22:45:19Z

Debug impl output is explicitly not stable and previous changes (#97176) didn't have libs-API approval either. I think could do without team approval.

thomcc · 2024-11-13T03:43:50Z

Fair enough! I was on the fence about it. It probably takes a t-libs FCP though. I think we should do it.

@rfcbot fcp merge

rfcbot · 2024-11-13T03:43:52Z

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

Concerns:

! (unix: Relax escaping in Debug impl on Command #132484 (comment))
use-single-quotes (unix: Relax escaping in Debug impl on Command #132484 (comment))

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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.

cuviper · 2024-11-13T17:19:14Z

In spirit, I'm fine with the change, however I think the special cases are incomplete. In particular, "2.4 Reserved Words" should be considered in that referenced doc, and I'm also surprised that ! (history expansion) isn't in the character list in general, but maybe that's more of a bash-ism.

But escape_ascii won't do the right thing for that either, because a command like echo "!foo" still attempts the expansion. We'd really need to use a proper shell escape routine. So I wonder what expectation we want to set here?

joshtriplett · 2024-11-13T19:26:39Z

library/std/src/sys/pal/unix/process/process_common.rs

+                        // As per documentation linked above:
+                        // > The application shall quote the following characters
+                        b'|' | b'&' | b';' | b'<' | b'>' | b'(' | b')' | b'$' | b'`' | b'\\'
+                        | b'"' | b'\'' | b' ' | b'\t' | b'\n' => false,


Could we get a test for the quoting of ", please?

Also, seconding the premise that we need to quote !.

Since this is UNIX-specific, I think it'd be reasonable to always use single-quotes for quoting, and just handle ' specially.

Could we get a test for the quoting of ", please?

There is a test for that? In library/std/src/sys/pal/unix/process/process_common/tests.rs:

check("a\"", r#""a\"""#);

Or perhaps I'm misunderstanding?

joshtriplett · 2024-11-13T19:27:28Z

@rfcbot reviewed

@rfcbot concern use-single-quotes
@rfcbot concern !

bors · 2024-11-24T13:26:54Z

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

madsmtm · 2024-11-25T09:37:57Z

So I wonder what expectation we want to set here?

I think we should say "best effort only" - we aren't going to support the user passing this to a shell automatically anyhow, so we don't really need to be worried about correctness IMO.

We'd really need to use a proper shell escape routine.

Though I could try to create one, if T-libs want to maintain it? ;)

Otherwise there's always shlex, we could use that too? (cc-rs already depends on it)

The Debug output of Command is very useful for showing to the user the command that was executed when something went wrong. This is done for example by rustc when invoking an external tool like the linker fails. It is also overly verbose, since everything is quoted, which makes it harder to read. Instead, we now first check if we're reasonably sure that an argument is simple enough that using it in the shell wouldn't need quoting, and then output it without quotes if possible. Before and example output could look like this: PATH="/a:/b" "cc" "foo.o" "-target" "arm64-apple-darwin11.0" Now it looks like this: PATH=/a:/b cc foo.o -target arm64-apple-darwin11.0

bors · 2024-12-15T14:25:33Z

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

the8472 · 2025-02-12T21:42:59Z

We discussed this during today's libs meeting. Since debug output is not stable we're ok with this change but would like to see the concerns addressed before merging it.

I think we should say "best effort only" - we aren't going to support the user passing this to a shell automatically anyhow, so we don't really need to be worried about correctness IMO.

The debug output makes an effort to be copy-pasteable since that's useful for manually replaying commands. Although it's not guaranteed behavior we'd like to preserve that property. This change appears to be compatible with that, but if issues get reported later we'd like them to be fixed. If this majorly breaks copy-pastability on common shells then reverting would also be on the table.

rustbot assigned thomcc Nov 2, 2024

rustbot added O-unix S-waiting-on-review T-libs labels Nov 2, 2024

madsmtm commented Nov 2, 2024

View reviewed changes

library/std/src/sys/pal/unix/process/process_common.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

the8472 reviewed Nov 2, 2024

View reviewed changes

library/std/src/sys/pal/unix/process/process_common/tests.rs Outdated Show resolved Hide resolved

madsmtm force-pushed the command-relaxed-escape branch from db8a0e7 to b45def2 Compare November 3, 2024 21:47

This comment has been minimized.

Sign in to view

madsmtm force-pushed the command-relaxed-escape branch from b45def2 to b0eb25f Compare November 3, 2024 22:01

This comment has been minimized.

Sign in to view

madsmtm force-pushed the command-relaxed-escape branch from b0eb25f to c28df60 Compare November 3, 2024 22:14

madsmtm requested a review from the8472 November 3, 2024 22:14

This comment has been minimized.

Sign in to view

madsmtm force-pushed the command-relaxed-escape branch from c28df60 to afb7d5f Compare November 4, 2024 01:09

rustbot added the A-run-make label Nov 4, 2024

bjorn3 reviewed Nov 6, 2024

View reviewed changes

library/std/src/process/tests.rs Show resolved Hide resolved

madsmtm force-pushed the command-relaxed-escape branch 2 times, most recently from e444b76 to a9c5e19 Compare November 11, 2024 06:22

This comment has been minimized.

Sign in to view

madsmtm force-pushed the command-relaxed-escape branch 2 times, most recently from b637ed5 to 55d3065 Compare November 11, 2024 06:41

thomcc added T-libs-api I-libs-api-nominated labels Nov 12, 2024

thomcc removed T-libs-api I-libs-api-nominated labels Nov 13, 2024

rfcbot added proposed-final-comment-period disposition-merge labels Nov 13, 2024

joshtriplett reviewed Nov 13, 2024

View reviewed changes

madsmtm force-pushed the command-relaxed-escape branch from 55d3065 to fd265af Compare November 25, 2024 09:31

This comment has been minimized.

Sign in to view

madsmtm force-pushed the command-relaxed-escape branch from fd265af to d83fe8d Compare November 25, 2024 09:59

thomcc added S-waiting-on-fcp and removed S-waiting-on-review labels Jan 14, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

unix: Relax escaping in `Debug` impl on `Command` #132484

unix: Relax escaping in `Debug` impl on `Command` #132484

madsmtm commented Nov 2, 2024 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rustbot commented Nov 4, 2024

This comment has been minimized.

thomcc commented Nov 12, 2024 •

edited

Loading

the8472 commented Nov 12, 2024

thomcc commented Nov 13, 2024

rfcbot commented Nov 13, 2024 •

edited

Loading

cuviper commented Nov 13, 2024

joshtriplett Nov 13, 2024

madsmtm Nov 25, 2024

joshtriplett commented Nov 13, 2024

bors commented Nov 24, 2024

madsmtm commented Nov 25, 2024 •

edited

Loading

This comment has been minimized.

bors commented Dec 15, 2024

the8472 commented Feb 12, 2025

unix: Relax escaping in Debug impl on Command #132484

Are you sure you want to change the base?

unix: Relax escaping in Debug impl on Command #132484

Conversation

madsmtm commented Nov 2, 2024 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rustbot commented Nov 4, 2024

This comment has been minimized.

thomcc commented Nov 12, 2024 • edited Loading

the8472 commented Nov 12, 2024

thomcc commented Nov 13, 2024

rfcbot commented Nov 13, 2024 • edited Loading

cuviper commented Nov 13, 2024

joshtriplett Nov 13, 2024

Choose a reason for hiding this comment

madsmtm Nov 25, 2024

Choose a reason for hiding this comment

joshtriplett commented Nov 13, 2024

bors commented Nov 24, 2024

madsmtm commented Nov 25, 2024 • edited Loading

This comment has been minimized.

bors commented Dec 15, 2024

the8472 commented Feb 12, 2025

unix: Relax escaping in `Debug` impl on `Command` #132484

unix: Relax escaping in `Debug` impl on `Command` #132484

madsmtm commented Nov 2, 2024 •

edited

Loading

thomcc commented Nov 12, 2024 •

edited

Loading

rfcbot commented Nov 13, 2024 •

edited

Loading

madsmtm commented Nov 25, 2024 •

edited

Loading