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

Add rustc --print rustc-path #100681

Closed
wants to merge 1 commit into from
Closed

Conversation

CAD97
Copy link
Contributor

@CAD97 CAD97 commented Aug 17, 2022

Related:

Goal:

Like the original rust-lang/rustup#2958, the goal is to enable cargo to call rustc directly, rather than through the rustup shim.

Solution:

cargo asks rustc to tell it what executable to run. Sometime early in compilation, cargo will run $(RUSTC_WRAPPER) $(RUSTC ?: rustc) --print rustc-path1. Further calls to rustc to do execution will use the resolved printed executable path rather than continuing to call the "input rustc path," which will allow it to bypass the rustup shim.

The cargo side is rust-lang/cargo#10998.

This is a tiny patch, so I've implemented it as insta-stable; this will need signoff probably from both the compiler and cargo teams. I'm working on the cargo side of the patch currently, but wanted to get this up ASAP.

cc @davidlattimore @bjorn3 @rust-lang/cargo @rust-lang/compiler (sorry for the big ping list 😅)

Footnotes

  1. This can potentially be combined with other --prints, as well!

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 17, 2022
@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 17, 2022
@CAD97

This comment was marked as outdated.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

Copy link
Contributor

@epage epage left a comment

Choose a reason for hiding this comment

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

At least from the cargo side of things

this will need signoff probably from both the compiler and cargo team

I would also like sign off from rustup folks on this being a viable alternative to rust-lang/rustup#3035 (ie they like it enough not to continue down that path, making this obsolete)

@CAD97
Copy link
Contributor Author

CAD97 commented Aug 17, 2022

@rustbot label +T-cargo +T-rustup

@CAD97
Copy link
Contributor Author

CAD97 commented Aug 17, 2022

@rustbot label +T-cargo

(there isn't a T-rustup)

@rustbot rustbot added the T-cargo Relevant to the cargo team, which will review and decide on the PR/issue. label Aug 17, 2022
@rust-log-analyzer

This comment has been minimized.

@petrochenkov
Copy link
Contributor

We need to either feature-gate this or start stabilization FCP.
(I don't have any preferences regarding this choice.)
@rustbot author

@rustbot rustbot 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 Aug 17, 2022
@CAD97
Copy link
Contributor Author

CAD97 commented Aug 17, 2022

Having it ungated for the time being helps for testing/measuring the rustup/cargo impact. I have a minor preference for just FCP to insta-stable, but that's FCP including the rustc, cargo, and rustup teams, that this approach is sufficient and desirable to resolve the overhead tracked in rust-lang/rustup#3035.

(Assuming it is sufficient, I think this is a more desirable way of handling this than any of the other proposed approaches, as it's "just" caching the shim's work rather than adding more environment variables and complicated rules for how they interact and override each other.)

@CAD97
Copy link
Contributor Author

CAD97 commented Aug 17, 2022

@rustbot review

I went ahead and feature gated this for the time being, so it can land a bit smoother.

@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 Aug 17, 2022
@petrochenkov
Copy link
Contributor

r=me after squashing commits.

@petrochenkov petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 17, 2022
@joshtriplett
Copy link
Member

Could this be combined with the rustc invocation that gets the rustc version, so that a single invocation gets both the version and the path? That would save one. Invocation.

@petrochenkov
Copy link
Contributor

@bors r-

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Aug 20, 2022
@CAD97
Copy link
Contributor Author

CAD97 commented Aug 21, 2022

nth try's the charm?

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Aug 21, 2022

📌 Commit f5be8a4 has been approved by petrochenkov

It is now in the queue for this repository.

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 21, 2022
@CAD97
Copy link
Contributor Author

CAD97 commented Aug 21, 2022

Could this be combined with the rustc invocation that gets the rustc version, so that a single invocation gets both the version and the path? [@joshtriplett]

I actually tried this, but that patch resulted in rustc actually responding slower for some reason.

  • Currently we do four calls to rustup-rustc at ~76ms each (three startup, one compilation);
  • with $env:RUSTC that's four calls to rustc at ~46ms each;
  • with rust-lang/cargo@e76de76 we make one call to rustup-rustc at ~76ms and four to rustc at ~51ms each;
  • with the alternative patch using --print verbose-version --print rustc-path, the call to rustup-rustc took >200ms assuming the following two calls to rustc took ~51ms each, or still >150ms if the following two calls took ~200ms.
attempt to rescale

The same calls to rustc taking a different duration with the same rustc doesn't sit well with me, so attempting to solve the system of equations

Solve[{
  4m + 4c + x = 0.303947129, (* 4 rustup, 4 rustc, 1 compilation *)
       4c + x = 0.185393026, (*           4 rustc, 1 compilation *)
   m + 5c + x = 0.278397067} (* 1 rustup, 5 rustc, 1 compilation *)
, x]

gave a solution with

  • rustup took 30ms each,
  • rustc took 63ms each, and
  • compilation took -68ms,

which doesn't make any sense, except to indicate that more changed than just the calls cargo is making.

I just though to control for cargo's overhead. I just measured cargo --version to take 0.012327236s (N=32) on the same commit; subtracting those from the targets as a fixed overhead (I was at least running development cargo directly in these, so no rustup overhead) gives

Solve[{
  4m + 4c + x = 0.291619893, (* 4 rustup, 4 rustc, 1 compilation *)
       4c + x = 0.17306579, (*           4 rustc, 1 compilation *)
   m + 5c + x = 0.266069831} (* 1 rustup, 5 rustc, 1 compilation *)
, x]
  • rustup took 30ms each
  • rustc took 63ms each, and
  • compilation took -80ms,

which in retrospect I guess makes sense, but makes the compilation take even more negative time >:(


(All timings from my machine, from significantly less than ideal testing, assuming that calls not doing a new --print are unaffected by the patches, and assuming compiling an empty bin takes no more time than just a --print, which is just surely false.)

The result with --print verbose-version --print rustc-path is so unexpected I don't trust it, but it was consistently repeatable when I was testing. The prospective patch is in the rustup thread if someone else wants to verify (and/or to note a silly mistake that's making the initial call 100+ms slower) and/or attack the problem with a finer toothed comb than I have available.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 21, 2022
…enkov

Add `rustc --print rustc-path`

Related:

- rust-lang/cargo#10986
- rust-lang/rustup#3035

Goal:

Like the original rust-lang/rustup#2958, the goal is to enable `cargo` to call `rustc` directly, rather than through the `rustup` shim.

Solution:

`cargo` asks `rustc` to tell it what executable to run. Sometime early in compilation, `cargo` will run `$(RUSTC_WRAPPER) $(RUSTC ?: rustc) --print rustc-path`[^1]. Further calls to `rustc` to do execution will use the resolved printed executable path rather than continuing to call the "input `rustc` path," which will allow it to bypass the `rustup` shim.

The cargo side is rust-lang/cargo#10998.

[^1]: This can potentially be combined with other `--print`s, as well!

This is a tiny patch, so I've implemented it as insta-stable; this will need signoff probably from both the compiler and cargo teams. I'm working on the cargo side of the patch currently, but wanted to get this up ASAP.

cc `@davidlattimore` `@bjorn3` `@rust-lang/cargo` `@rust-lang/compiler` (sorry for the big ping list 😅)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 21, 2022
…enkov

Add `rustc --print rustc-path`

Related:

- rust-lang/cargo#10986
- rust-lang/rustup#3035

Goal:

Like the original rust-lang/rustup#2958, the goal is to enable `cargo` to call `rustc` directly, rather than through the `rustup` shim.

Solution:

`cargo` asks `rustc` to tell it what executable to run. Sometime early in compilation, `cargo` will run `$(RUSTC_WRAPPER) $(RUSTC ?: rustc) --print rustc-path`[^1]. Further calls to `rustc` to do execution will use the resolved printed executable path rather than continuing to call the "input `rustc` path," which will allow it to bypass the `rustup` shim.

The cargo side is rust-lang/cargo#10998.

[^1]: This can potentially be combined with other `--print`s, as well!

This is a tiny patch, so I've implemented it as insta-stable; this will need signoff probably from both the compiler and cargo teams. I'm working on the cargo side of the patch currently, but wanted to get this up ASAP.

cc ``@davidlattimore`` ``@bjorn3`` ``@rust-lang/cargo`` ``@rust-lang/compiler`` (sorry for the big ping list 😅)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 21, 2022
…enkov

Add `rustc --print rustc-path`

Related:

- rust-lang/cargo#10986
- rust-lang/rustup#3035

Goal:

Like the original rust-lang/rustup#2958, the goal is to enable `cargo` to call `rustc` directly, rather than through the `rustup` shim.

Solution:

`cargo` asks `rustc` to tell it what executable to run. Sometime early in compilation, `cargo` will run `$(RUSTC_WRAPPER) $(RUSTC ?: rustc) --print rustc-path`[^1]. Further calls to `rustc` to do execution will use the resolved printed executable path rather than continuing to call the "input `rustc` path," which will allow it to bypass the `rustup` shim.

The cargo side is rust-lang/cargo#10998.

[^1]: This can potentially be combined with other `--print`s, as well!

This is a tiny patch, so I've implemented it as insta-stable; this will need signoff probably from both the compiler and cargo teams. I'm working on the cargo side of the patch currently, but wanted to get this up ASAP.

cc ```@davidlattimore``` ```@bjorn3``` ```@rust-lang/cargo``` ```@rust-lang/compiler``` (sorry for the big ping list 😅)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 21, 2022
…enkov

Add `rustc --print rustc-path`

Related:

- rust-lang/cargo#10986
- rust-lang/rustup#3035

Goal:

Like the original rust-lang/rustup#2958, the goal is to enable `cargo` to call `rustc` directly, rather than through the `rustup` shim.

Solution:

`cargo` asks `rustc` to tell it what executable to run. Sometime early in compilation, `cargo` will run `$(RUSTC_WRAPPER) $(RUSTC ?: rustc) --print rustc-path`[^1]. Further calls to `rustc` to do execution will use the resolved printed executable path rather than continuing to call the "input `rustc` path," which will allow it to bypass the `rustup` shim.

The cargo side is rust-lang/cargo#10998.

[^1]: This can potentially be combined with other `--print`s, as well!

This is a tiny patch, so I've implemented it as insta-stable; this will need signoff probably from both the compiler and cargo teams. I'm working on the cargo side of the patch currently, but wanted to get this up ASAP.

cc ````@davidlattimore```` ````@bjorn3```` ````@rust-lang/cargo```` ````@rust-lang/compiler```` (sorry for the big ping list 😅)
@matthiaskrgr
Copy link
Member

@bors r- failed in a rollup
#100840 (comment)

@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 Aug 21, 2022

ifdef IS_WINDOWS
all:
$(RUSTC) -Zunstable-options --print rustc-path | $(CGREP) bin\rustc
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Looks like \ needs to be escaped here.
  2. FWIW, CGREP supports regexes to match on both \ and / together.

@bors
Copy link
Contributor

bors commented Nov 2, 2022

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

@nagisa
Copy link
Member

nagisa commented Nov 3, 2022

Is this not pretty much the same as $(rustc --print=sysroot)/bin/rustc? Raising this point as I don’t see any discussion to this effect, and it isn’t obvious that these are not equivalent (if they aren’t)

@bjorn3
Copy link
Member

bjorn3 commented Nov 3, 2022

If RUSTFLAGS contains --sysroot foo then cargo would run rustc --sysroot foo --print sysroot, which will return foo even though rustc is likely not in this sysroot.

@CAD97
Copy link
Contributor Author

CAD97 commented Nov 22, 2022

I'm closing this rather than fix the merge conflict, as I don't have the bandwidth to push the cargo side of the change. There's not a consensus on the cargo side that the extra startup call to rustc to strip the rustup shim, and my previous attempt to allow batching --print rustc-path with -Vv had the opposite perf effect, likely due to how early -Vv is handled compared to --print.

@CAD97 CAD97 closed this Nov 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-cargo Relevant to the cargo team, which will review and decide on the PR/issue. 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.