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

Respect -o output file in --print #110785

Closed
wants to merge 8 commits into from
Closed

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Apr 25, 2023

Previously something like rustc --print=cfg -o cfg.txt just ignored the -o option and would print to stdout as usual.

File output simplifies integration in non-Cargo build systems — in my case Buck2. With this PR we get to skip a layer of Python wrapper or Bash wrapper to redirect rustc's stdout into the location dictated by the build system.

@rustbot
Copy link
Collaborator

rustbot commented Apr 25, 2023

r? @jackh726

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 25, 2023
@Mark-Simulacrum Mark-Simulacrum added the relnotes Marks issues that should be documented in the release notes of the next release. label Apr 25, 2023
@dtolnay
Copy link
Member Author

dtolnay commented Apr 25, 2023

@jyn514
Copy link
Member

jyn514 commented Apr 26, 2023

This makes sense to me, since we currently abort compilation early if you pass --print:

; rustc main.rs -o foo --print=target-list >/dev/null; file foo
foo: cannot open `foo' (No such file or directory)

That said, are we sure we want to commit to that behavior? Could there be a scenario where it makes sense to emit the --print info but still continue compilation? In that case, the -o argument would cause a collision, we'd try and output the binary and the print info to the same file.

@apiraino
Copy link
Contributor

r? compiler

@rustbot rustbot assigned wesleywiser and unassigned jackh726 May 17, 2023
@apiraino
Copy link
Contributor

apiraino commented Jun 7, 2023

(sorry for the pings)

r? compiler

@rustbot rustbot assigned fee1-dead and unassigned wesleywiser Jun 7, 2023
@fee1-dead fee1-dead 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 Jun 7, 2023
@dtolnay dtolnay 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 13, 2023
@dtolnay dtolnay 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 Jul 14, 2023
@fee1-dead
Copy link
Member

Thanks. Could you add an ui test for the llvm print changes? After that r=me.

@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 Jul 15, 2023
@bors
Copy link
Contributor

bors commented Jul 15, 2023

⌛ Testing commit 89a32a4 with merge 6cd0af32cea94ac27cc4e4135acdd5b7e111ee53...

@bors
Copy link
Contributor

bors commented Jul 15, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 15, 2023
@rust-log-analyzer
Copy link
Collaborator

The job armhf-gnu failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
curl: (22) The requested URL returned error: 404 

error: failed to download llvm from ci

    help: old builds get deleted after a certain time
    help: if trying to compile an old commit of rustc, disable `download-ci-llvm` in config.toml:
    [llvm]
    download-ci-llvm = false
    
Build completed unsuccessfully in 0:00:00

@jyn514
Copy link
Member

jyn514 commented Jul 15, 2023

@bors retry #113713

@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 Jul 15, 2023
@bjorn3
Copy link
Member

bjorn3 commented Jul 15, 2023

Doesn't this break -o foo --emit=link --print native-static-libs? Both --emit=link and --print native-static-libs would be fighting over writing foo.

@jyn514
Copy link
Member

jyn514 commented Jul 15, 2023

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 15, 2023
@bjorn3
Copy link
Member

bjorn3 commented Jul 15, 2023

Maybe --print could be changed to allow explicitly specifying a path just like for --emit?

@dtolnay
Copy link
Member Author

dtolnay commented Jul 15, 2023

Doesn't this break -o foo --emit=link --print native-static-libs? Both --emit=link and --print native-static-libs would be fighting over writing foo.

Good question. This PR does not apply to native-static-libs. That's handled differently from all the other --print options: --print normally means print to stdout, whereas --print=native-static-libs gets emitted via diagnostics machinery instead (sess.emit_note) to stderr, and continues to work that way after this PR. As far as I can tell the intended way to use native-static-libs is in combination with --error-format=json and then searching for a note with prefix "native-static-libs:" among the JSON messages.

So this PR does not break it or cause fighting over writing the same output file with multiple contents. native-static-libs will be inconsistent vs the other --print options by not respecting -o, but has always been inconsistent already, so I'm not sure it ends up being worse.


Maybe --print could be changed to allow explicitly specifying a path just like for --emit?

I am open to this. --print sysroot=sysroot.txt --print cfg=cfg.txt

I think -o makes sense to apply to --print regardless of whether --print KIND=PATH is supported. -o means "set a location for the output that was asked for". In rustc --emit=metadata -o PATH, the -o means set the location for the rmeta output. In rustc --emit=asm -o PATH, the -o means set the location for the asm output. In rustc --print=cfg -o PATH, the configuration data is the output being asked for, and I think -o should apply.

If --print + -o does get supported, I'm not sure I would be able to justify --print KIND=PATH in addition. @bjorn3 do you feel that -o should not apply to --print, or do you feel that both --print + -o and --print KIND=PATH should work?

I can see that for native-static-libs, rustc --emit link=libfoo.a --print native-static-libs=native-static-libs.txt can be potentially more convenient than grabbing notes from a JSON diagnostic.

@bjorn3
Copy link
Member

bjorn3 commented Jul 15, 2023

This PR does not apply to native-static-libs.

Right. It also doesn't apply to --print link-args it seems even though that one doesn't use a diagnostic. That one doesn't do an early exit unlike most --print either.

If --print + -o does get supported, I'm not sure I would be able to justify --print KIND=PATH in addition. @bjorn3 do you feel that -o should not apply to --print, or do you feel that both --print + -o and --print KIND=PATH should work?

I think --print + -o should not be supported. For --emit + -o rustc changes the file extension if there are multiple outputs. For --print there are no obvious file extensions to use. I don't have any strong objection though. If it does get supported I think both --print + -o and --print KIND=PATH should be supported.

I can see that for native-static-libs, rustc --emit link=libfoo.a --print native-static-libs=native-static-libs.txt can be potentially more convenient than grabbing notes from a JSON diagnostic.

Indeed.

@dtolnay dtolnay 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 16, 2023
Copy link
Member Author

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Closing in favor of --print KIND=PATH#113780. Thank you for the review! I agree that is a better approach.

@dtolnay dtolnay closed this Jul 17, 2023
@dtolnay dtolnay deleted the printoutfile branch July 17, 2023 06:33
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 20, 2023
Support `--print KIND=PATH` command line syntax

As is already done for `--emit KIND=PATH` and `-L KIND=PATH`.

In the discussion of rust-lang#110785, it was pointed out that `--print KIND=PATH` is nicer than trying to apply the single global `-o` path to `--print`'s output, because in general there can be multiple print requests within a single rustc invocation, and anyway `-o` would already be used for a different meaning in the case of `link-args` and `native-static-libs`.

I am interested in using `--print cfg=PATH` in Buck2. Currently Buck2 works around the lack of support for `--print KIND=PATH` by [indirecting through a Python wrapper script](https://github.com/facebook/buck2/blob/d43cf3a51a31f00be2c2248e78271b0fef0452b4/prelude/rust/tools/get_rustc_cfg.py) to redirect rustc's stdout into the location dictated by the build system.

From skimming Cargo's usages of `--print`, it definitely seems like it would benefit from `--print KIND=PATH` too. Currently it is working around the lack of this by inserting `--crate-name=___ --print=crate-name` so that it can look for a line containing `___` as a delimiter between the 2 other `--print` informations it actually cares about. This is commented as a "HACK" and "abuse". https://github.com/rust-lang/cargo/blob/31eda6f7c360d9911f853b3014e057db61238f3e/src/cargo/core/compiler/build_context/target_info.rs#L242 (FYI `@weihanglo` as you dealt with this recently in rust-lang/cargo#11633.)

Mentioning reviewers active in rust-lang#110785: `@fee1-dead` `@jyn514` `@bjorn3`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 21, 2023
Support `--print KIND=PATH` command line syntax

As is already done for `--emit KIND=PATH` and `-L KIND=PATH`.

In the discussion of rust-lang#110785, it was pointed out that `--print KIND=PATH` is nicer than trying to apply the single global `-o` path to `--print`'s output, because in general there can be multiple print requests within a single rustc invocation, and anyway `-o` would already be used for a different meaning in the case of `link-args` and `native-static-libs`.

I am interested in using `--print cfg=PATH` in Buck2. Currently Buck2 works around the lack of support for `--print KIND=PATH` by [indirecting through a Python wrapper script](https://github.com/facebook/buck2/blob/d43cf3a51a31f00be2c2248e78271b0fef0452b4/prelude/rust/tools/get_rustc_cfg.py) to redirect rustc's stdout into the location dictated by the build system.

From skimming Cargo's usages of `--print`, it definitely seems like it would benefit from `--print KIND=PATH` too. Currently it is working around the lack of this by inserting `--crate-name=___ --print=crate-name` so that it can look for a line containing `___` as a delimiter between the 2 other `--print` informations it actually cares about. This is commented as a "HACK" and "abuse". https://github.com/rust-lang/cargo/blob/31eda6f7c360d9911f853b3014e057db61238f3e/src/cargo/core/compiler/build_context/target_info.rs#L242 (FYI `@weihanglo` as you dealt with this recently in rust-lang/cargo#11633.)

Mentioning reviewers active in rust-lang#110785: `@fee1-dead` `@jyn514` `@bjorn3`
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 7, 2023
…li-obk

Stabilize `PATH` option for `--print KIND=PATH`

This PR propose stabilizing the `PATH` option for `--print KIND=PATH`. This option was previously added in rust-lang#113780 (as insta-stable before being un-stablized in rust-lang#114139).

Description of the `PATH` option:
> A filepath may optionally be specified for each requested information kind, in the format `--print KIND=PATH`, just like for `--emit`. When a path is specified, information will be written there instead of to stdout.

------

Description of the original PR [\[link\]](rust-lang#113780 (comment)):
> **Support --print KIND=PATH command line syntax**
>
> As is already done for `--emit KIND=PATH` and `-L KIND=PATH`.
>
> In the discussion of rust-lang#110785, it was pointed out that `--print KIND=PATH` is nicer than trying to apply the single global `-o path` to `--print`'s output, because in general there can be multiple print requests within a single rustc invocation, and anyway `-o` would already be used for a different meaning in the case of `link-args` and `native-static-libs`.
>
> I am interested in using `--print cfg=PATH` in Buck2. Currently Buck2 works around the lack of support for `--print KIND=PATH` by [indirecting through a Python wrapper script](https://github.com/facebook/buck2/blob/d43cf3a51a31f00be2c2248e78271b0fef0452b4/prelude/rust/tools/get_rustc_cfg.py) to redirect rustc's stdout into the location dictated by the build system.
>
> From skimming Cargo's usages of `--print`, it definitely seems like it would benefit from `--print KIND=PATH` too. Currently it is working around the lack of this by inserting `--crate-name=___ --print=crate-name` so that it can look for a line containing `___` as a delimiter between the 2 other `--print` informations it actually cares about. This is commented as a "HACK" and "abuse". https://github.com/rust-lang/cargo/blob/31eda6f7c360d9911f853b3014e057db61238f3e/src/cargo/core/compiler/build_context/target_info.rs#L242

-----

cc `@dtolnay`
r? `@jackh726`
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Sep 12, 2023
Stabilize `PATH` option for `--print KIND=PATH`

This PR propose stabilizing the `PATH` option for `--print KIND=PATH`. This option was previously added in rust-lang/rust#113780 (as insta-stable before being un-stablized in rust-lang/rust#114139).

Description of the `PATH` option:
> A filepath may optionally be specified for each requested information kind, in the format `--print KIND=PATH`, just like for `--emit`. When a path is specified, information will be written there instead of to stdout.

------

Description of the original PR [\[link\]](rust-lang/rust#113780 (comment)):
> **Support --print KIND=PATH command line syntax**
>
> As is already done for `--emit KIND=PATH` and `-L KIND=PATH`.
>
> In the discussion of rust-lang/rust#110785, it was pointed out that `--print KIND=PATH` is nicer than trying to apply the single global `-o path` to `--print`'s output, because in general there can be multiple print requests within a single rustc invocation, and anyway `-o` would already be used for a different meaning in the case of `link-args` and `native-static-libs`.
>
> I am interested in using `--print cfg=PATH` in Buck2. Currently Buck2 works around the lack of support for `--print KIND=PATH` by [indirecting through a Python wrapper script](https://github.com/facebook/buck2/blob/d43cf3a51a31f00be2c2248e78271b0fef0452b4/prelude/rust/tools/get_rustc_cfg.py) to redirect rustc's stdout into the location dictated by the build system.
>
> From skimming Cargo's usages of `--print`, it definitely seems like it would benefit from `--print KIND=PATH` too. Currently it is working around the lack of this by inserting `--crate-name=___ --print=crate-name` so that it can look for a line containing `___` as a delimiter between the 2 other `--print` informations it actually cares about. This is commented as a "HACK" and "abuse". https://github.com/rust-lang/cargo/blob/31eda6f7c360d9911f853b3014e057db61238f3e/src/cargo/core/compiler/build_context/target_info.rs#L242

-----

cc `@dtolnay`
r? `@jackh726`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.