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

Chore: Refactor use of cargo-install #279

Closed
NobodyXu opened this issue Aug 4, 2022 · 8 comments
Closed

Chore: Refactor use of cargo-install #279

NobodyXu opened this issue Aug 4, 2022 · 8 comments
Labels
Report: enhancement Request for improvements to existing features or code

Comments

@NobodyXu
Copy link
Member

NobodyXu commented Aug 4, 2022

From #251 (comment) :

Upstream jobserver has a longstanding bug relating to lifetime of Client alexcrichton/jobserver-rs#25

I submit a solution for fixing this alexcrichton/jobserver-rs#40 which unfortunately was rejected due to changing of its interface.

While use of jobserver in cargo-binstall is currently ok and does not trigger the bug, I would still like it to be fixed and the PR I mentioned also improve performance of spawning a new process since it can use posix_spawn on unix (which can use vfork on linux).

Regarding this, I figured out that we don't need to use jobserver at all.
We could just use the batch installation feature of cargo-install:

cargo install $crate_name@$version

Though that will not result in parallel compilation of multiple binary crates rust-lang/cargo#9741

@NobodyXu NobodyXu added the Report: enhancement Request for improvements to existing features or code label Aug 4, 2022
@NobodyXu NobodyXu self-assigned this Aug 4, 2022
@passcod
Copy link
Member

passcod commented Aug 4, 2022

I think keeping track of where each crate is at could still be valuable, but we could keep it serial like in the UI PR rather than parallel with jobserver. The advantage of parallel cargo installs with jobserver is we do avoid some of the "final crate bottleneck" but it might not be worth it.

@NobodyXu
Copy link
Member Author

NobodyXu commented Aug 4, 2022

I think keeping track of where each crate is at could still be valuable, but we could keep it serial like in the UI PR rather than parallel with jobserver. The advantage of parallel cargo installs with jobserver is we do avoid some of the "final crate bottleneck" but it might not be worth it.

Using batch installation is just the same actually, since all we need to do is to launch one command:

cargo install $crate_name1@$version1 $crate_name2@$version2 ...

@passcod
Copy link
Member

passcod commented Aug 4, 2022

Yeah but UI wise I want to hide the cargo install output and just show the progress.

Though, hmm, maybe what would be better in that case is to use json output and the single command, and parse the output for UI purposes.

@passcod
Copy link
Member

passcod commented Aug 4, 2022

Ah, json output is unstable :(

Ok, let's do a single command for now and I'll figure out something more advanced later if I can

@laoshaw
Copy link

laoshaw commented Sep 17, 2022

when cargo install pkg1 pkg2 pkg3 there are many components are the same, cargo install just build them again and again and all in serial sequence, can cargo-install them in parallel, if possible even reuse those shared component to avoid recompiling?

when I cargo install, only one core is busy and the rest cores are idle, which is, slow

@NobodyXu
Copy link
Member Author

can cargo-install them in parallel

There's a ticket for it in cargo rust-lang/cargo#9741
I wished it'd support this but the issue has been inactive for a long time.

if possible even reuse those shared component to avoid recompiling?

That's also a great idea since many crates share the same dependency and they do not override profile.
But you would have to open the issue on rust-lang/cargo as we cannot fix the issue here.

@NobodyXu
Copy link
Member Author

@laoshaw Just a note that this hasn't been implemented yet and as of v0.13.1, we still spawn multiple cargo-install instances for every crate, so no worry now.

I would probably wait until json output is stablised and rust-lang/cargo#9741 is implemented.

@NobodyXu
Copy link
Member Author

I am settled on our current implementation of launching cargo-install after installing all pre-built binaries and having multiple cargo-install instances to speedup compilation.

@NobodyXu NobodyXu closed this as not planned Won't fix, can't repro, duplicate, stale Feb 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Report: enhancement Request for improvements to existing features or code
Projects
None yet
Development

No branches or pull requests

3 participants