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

Stop defaulting to $PATH searches when the binary can't be found and causing infinite recursion #917

Merged
merged 4 commits into from
Jan 26, 2017

Conversation

Manishearth
Copy link
Member

I don't think we should ever be falling back to letting the OS resolve on the basis of $PATH, but if others still want that functionality I can make this error happen only in the recursion case (when RUST_RECURSION_COUNT is 4 or something).

Currently if you have a broken toolchain (e.g. a rustup link toolchain to an in-progress build directory that doesn't have the executables in it yet) rustup will default to a $PATH search when it can't find the binary, and this will just resolve back to rustup, which causes the infinite recursion error.

Took me a while to figure out that this was a bug with rustup and not my code. Someone else hit it today as well when trying to use rust-gdb, so it might have been introduced recently, though that doesn't seem to be the case.

r? @brson

@Diggsey
Copy link
Contributor

Diggsey commented Jan 9, 2017

The PATH fallback is important functionality in rustup - it's used whenever using a custom toolchain without cargo, and there are several other cases where you want to run a program in the environment for a particular toolchain.

@Manishearth
Copy link
Member Author

I don't see how that ever worked? If you have a toolchain without cargo the PATH fallback will just re-resolve to rustup and go in cycles. This works using env vars so unless we explicitly handle this it won't work.

I think we should change this code to explicitly try the default toolchain instead (and emit a warning that it did a fallback), and not do PATH-resolution at all.

@Manishearth
Copy link
Member Author

Or, it's only going to work for binaries that themselves don't shell out to rustup. The test that handles this does rustup run nightly sh, which will work.

@Manishearth
Copy link
Member Author

I made it check the recursion var so that rustup run nightly sh foo still works.

@petrochenkov
Copy link
Contributor

@Diggsey

The PATH fallback is important functionality in rustup - it's used whenever using a custom toolchain without cargo

Is this what happens when you e.g. grab Rust sources, build your own stage1 compiler from them, and use it as a custom toolchain for building Cargo-based projects?
(In this case it is indeed important.)

@Manishearth
Copy link
Member Author

Is this what happens when you e.g. grab Rust sources, build your own stage1 compiler from them, and use it as a custom toolchain for building Cargo-based projects?

That's what should happen, but it doesn't work right now.

@Diggsey
Copy link
Contributor

Diggsey commented Jan 9, 2017

Was on my phone so couldn't give much detail earlier.

This is the PR that added the fallback: #822
As you can see it fixed two separate issues, #641 and #210 - the cargo issue may be me mis-remembering, or it could be that PATH semantics have changed since then...

Thanks for making the change anyway, however I think:

  • It should be consistent with how RUST_RECURSION_COUNT is used elsewhere (where a value of 5 is used)
  • It should not be fixed at a very low value, because it will give false positives when tools call each other (eg. cargo calling rustc goes via rustup again)
  • If it's going to have a default low value, it should at least be possible to override via another environment variable.

@Manishearth
Copy link
Member Author

It has to be less than 5; since it needs to preempt the regular recursion check. I can make it 4.

@Diggsey
Copy link
Contributor

Diggsey commented Jan 9, 2017

Can you make it a named constant, use <constant> - 1 in this case, and add a regression test.

Rustup has a big problem with changes to one aspect of the tool causing problems elsewhere, and the only way it's going to get better is by making dependencies more explicit, and being meticulous about adding regression tests.

@brson
Copy link
Contributor

brson commented Jan 24, 2017

This PR looks good to me, but there is a legit test failure.

Edit: thanks @Manishearth and @Diggsey

@Manishearth
Copy link
Member Author

The test failure arises because we're trying to test what happens when you fallback to the path in a regular install, and the test environment is not a regular install -- it has multiple rustcs (one in the path from Travis, one that goes through rustup). @Diggsey said he'd look into fixing this IIRC. I don't recall what solution we'd come up with.

@Diggsey
Copy link
Contributor

Diggsey commented Jan 24, 2017

@Manishearth sorry, lost track of this - I think I have a better way to get this to work rather than messing too much with the CI environment, I'm just testing it out now.

@Diggsey
Copy link
Contributor

Diggsey commented Jan 25, 2017

I decided to temporarily hardlink a new proxy binary to use for the test. This seems to work for the most part, although some of the linux tests are still failing, as they don't seem to pick it up.

@brson
Copy link
Contributor

brson commented Jan 25, 2017

Yeah that looks like a good fix in the test. Nice thinking.

@Diggsey
Copy link
Contributor

Diggsey commented Jan 26, 2017

Travis has decided not to run my build, and my local reproduction of the problem fixed itself before I even changed anything 😡 Who knows, it might be fixed now...

@brson brson merged commit 4029c16 into rust-lang:master Jan 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants