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

involve rustup when determining toolchain paths #666

Merged
merged 18 commits into from
Mar 20, 2020
Merged

involve rustup when determining toolchain paths #666

merged 18 commits into from
Mar 20, 2020

Conversation

drahnr
Copy link
Collaborator

@drahnr drahnr commented Feb 17, 2020

Start of impl to handle #87

@drahnr
Copy link
Collaborator Author

drahnr commented Feb 17, 2020 •
edited
Loading

Does not cover the case cargo +nightly b which is due to a missing mtime in the proxy cache.
Workspaces with different toolchains (i.e. with https://github.com/paritytech/substrate ) don't work yet either, possible due to the same root cause.

@drahnr drahnr force-pushed the master branch 2 times, most recently from 5c589b8 to 6a18f86 Compare February 17, 2020 17:33
@drahnr drahnr changed the title involve rustup when determining toolchain paths, part 1, incomplete involve rustup when determining toolchain paths Feb 17, 2020
@drahnr
Copy link
Collaborator Author

drahnr commented Feb 17, 2020 •
edited
Loading

Is the restriction to rsut 1.36 necessary? That's why the CI fails since Option<Option<_>> does not yet have the flatten.

@drahnr
Copy link
Collaborator Author

drahnr commented Feb 18, 2020

use a lookup for the compiler_path to a Box<dyn CompilerProxy> which is used for the exact compiler lookup depending on the cwd current working dir.
RustupProxy runs rustup which rustc and creates a Rust / Box<dyn Compiler> instance.
This information is then stored with modifaction time stored as if the Proxy never existed.

Current ugliness:

  • compiler type needs to be known so the correct proxy can be picked up - this is all mingled in detect_compiler right now, which returns a Rust and RustupProxy (or more precisely: (Box<dyn Compiler>, Option<Box<dyn CompilerProxy>>)

@drahnr
Copy link
Collaborator Author

drahnr commented Feb 20, 2020

This was more work than anticipated and the futures pre-stabilization caused quite a few not anticipated changes.
I'd like some feedback on the allover idea, and maybe on how to reduce the clutter a bit.

@drahnr drahnr requested a review from froydnj February 21, 2020 10:08
@drahnr
Copy link
Collaborator Author

drahnr commented Feb 21, 2020

Verified this at least solves #663 .

@drahnr drahnr force-pushed the master branch 2 times, most recently from 650f167 to b1de993 Compare February 26, 2020 09:51
@drahnr drahnr mentioned this pull request Feb 28, 2020
Open
chmanchester
chmanchester reviewed Feb 28, 2020
View reviewed changes
Copy link
Contributor

@chmanchester chmanchester left a comment

Choose a reason for hiding this comment

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

Thank you for taking on this change, I think we certainly want to take something like this, although I agree it would be nice to be able to simplify the implementation. froydnj might be able to provide a more detailed review.

@drahnr
Copy link
Collaborator Author

drahnr commented Mar 4, 2020

@froydnj if you could give some feedback so I can move this forward, that'd be great

froydnj
froydnj reviewed Mar 4, 2020
View reviewed changes
Copy link
Contributor

@froydnj froydnj left a comment

Choose a reason for hiding this comment

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

I do not have brilliant ideas on how to simplify things right now: the cache structure is necessarily more complicated because of rustup. Some comments below.

src/server.rs Outdated Show resolved Hide resolved
src/server.rs Outdated Show resolved Hide resolved
src/server.rs Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
src/compiler/compiler.rs Outdated Show resolved Hide resolved
src/server.rs Outdated Show resolved Hide resolved
src/server.rs Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/compiler/compiler.rs Outdated Show resolved Hide resolved
@drahnr drahnr force-pushed the master branch 5 times, most recently from b77e4bd to a77e0d3 Compare March 5, 2020 09:47
@drahnr drahnr mentioned this pull request Mar 5, 2020
Closed
@drahnr
Copy link
Collaborator Author

drahnr commented Mar 5, 2020

@froydnj I addressed all concerns and verified it works using spearow/juice as a demo project - there are still 2 open comments :)

@drahnr drahnr requested a review from froydnj March 9, 2020 09:35
@drahnr
Copy link
Collaborator Author

drahnr commented Mar 9, 2020

@froydnj can we clarify the last point, (either implement one of both options or merge this as is) unless there are any remaining issues? that'd be much appreciated - thanks!

@drahnr
Copy link
Collaborator Author

drahnr commented Mar 11, 2020

@froydnj @luser I implemented a hybrid solution of what both you suggested, after all they are not really exclusive and work very well hand in hand. Given the fact that it is only run once per task on the client, the amount of additional work required should be negligible since it's cached for subsequent compiles.

@drahnr
Copy link
Collaborator Author

drahnr commented Mar 11, 2020 •
edited
Loading

b4aabf0 was added to make +beta happy, also part of #683

drahnr and others added 18 commits March 19, 2020 08:52
@drahnr
involve rustup when determining toolchain paths, part 1
8a0ddcf
@drahnr
test: add newly required cwd argument
10ccb70
@drahnr
chore/test: remove unnecessary clones
0b172ac
@drahnr
refactor/new: remove from impl in favour of new impl
b129775
@drahnr
fix/rustup/cwd: adjust cwd of proxy resolution command
9b6e487
@drahnr
chore/cleanup: remove unused trait fn
5fa0d97
@drahnr
chore/env: requires static lifetime, so clone
d10957f
@drahnr
chore/deps: bump which to version 3 and do not depend on failure
c3d10db
@drahnr
chore/rust/errors: better error messages for rustup
4ad92bd
@drahnr
chore/compiler_proxy: use scope instead of explicit drop() call
ecf9fde
@drahnr
docs/compiler_proxy: reword misleading comment
6faf366
@drahnr
fix/test: add additional mock children for rustc detection
8842529
@drahnr
feat/rustup/detect: improve detection code for rustup
034c3a4
@drahnr
chore/rustup/log: improve logging consistency
4385606
@drahnr
chore/fix: nits, spelling
b895f89
@drahnr
refactor/proxy: remove Option wrapping from proxy resolve, add additi…
843fa10 
…onal comments
@drahnr
chore/doc: trivial doc on CompilerProxy
c17b818
@drahnr
chore/compiler: run cargo fix
d54c67d
@drahnr
Copy link
Collaborator Author

drahnr commented Mar 19, 2020

Rebased, removed obsolete changes and resolved all review comments.

This was referenced Mar 19, 2020
Closed
Closed
@froydnj froydnj merged commit 1351a07 into mozilla:master Mar 20, 2020
sthagen added a commit to sthagen/mozilla-sccache that referenced this pull request Mar 22, 2020
@sthagen
Merge pull request #6 from mozilla/master
1142493 
involve rustup when determining toolchain paths (mozilla#666)
@luser luser mentioned this pull request Sep 17, 2020
Closed
@drahnr drahnr mentioned this pull request Sep 28, 2020
Closed
@drahnr drahnr mentioned this pull request Nov 11, 2020
Closed
@drahnr drahnr mentioned this pull request Apr 13, 2021
Merged
@figsoda figsoda mentioned this pull request Oct 20, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chmanchester chmanchester chmanchester left review comments

@luser luser luser left review comments

@froydnj froydnj Awaiting requested review from froydnj

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@drahnr @froydnj @luser @chmanchester