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

[PoC] Replace libcargo with cargo CLI #70

Merged
merged 6 commits into from
Oct 21, 2022
Merged

Conversation

oxalica
Copy link
Contributor

@oxalica oxalica commented Oct 19, 2022

Fixes #69

This PR is an incomplete draft for more discussion.

  • --lib, --test, --bench, --package selectors, and auto-selection are implemented. Asm paths are constructed precisely without guessing.
  • --bin, --example are not implemented, which need file searching and guessing. We could construct asm paths using unstable --build-plan arguments but not sure that's acceptable.
    --bin, --example are implemented by tracking hard-links to get the asm file name.
  • --release, --dry-run and etc flags should be trivial by passing-through. They are not implemented yet to focus on the overall approach. Implemented.

Currently we use some kind of guessing for extra-filenames (the hash part in filename), which may need cargo clean before the run. It's kind of annoying to clean artifacts for a big project.

@pacak
Copy link
Owner

pacak commented Oct 19, 2022

While I'm not too bothered by compilation time of the cargo-show-asm itself - I'm not opposed to idea making it faster if we can achieve feature parity :)

Main motivation to use cargo as a library was to make sure that as long as it compiles - it does roughly the same things as cargo itself.

I tried to compiling cargo-show-asm from your branch, it seems to work but cargo-show-asm is not able to reuse compilation results from cargo build calls
Current behavior:

$ cargo clean
$ cargo build --release -p foo # this will compile foo and everything it needs
$ cargo asm -p foo bar         # this will compile only foo

Behavior with cargo as a cli:

$ cargo clean
$ cargo build --release -p foo # this will compile foo and everything it needs
$ cargo asm -p foo bar         # this will compile foo and everything it needs

In my case it's up to extra ~ 900 crates to compile and takes a while. Subsequent calls to cargo asm do reuse it so it's one time cost but so is compiling libcargo.

I think this is caused by cargo using slightly different compilation options - and there comes a more fundamental problem - even if we get those options to match now they might go out of sync later with newer versions. Any ideas how can this be fixed in a future proof way?

@pacak
Copy link
Owner

pacak commented Oct 19, 2022

which may need cargo clean before the run. It's kind of annoying to clean artifacts for a big project.

Current version uses cargo clean equivalent but only on a crate it is currently trying to compile, mildly annoying but not a huge deal.

@oxalica
Copy link
Contributor Author

oxalica commented Oct 19, 2022

@pacak

Main motivation to use cargo as a library was to make sure that as long as it compiles - it does roughly the same things as cargo itself.

You use "roughly" and that's true. The bundled libcargo may diverge with user's cargo, leading to potential misbehavior or collision. That can be much complicated when mixing stable cargo and nightly one (and may with features enabled).

I tried to compiling cargo-show-asm from your branch, it seems to work but cargo-show-asm is not able to reuse compilation results from cargo build calls

Oh, that's because I don't forward --release flag yet. So it uses debug build by default. I can verify that cargo clean && cargo build && cargo asm do reuse all dependencies. Only the final binaries are rebuilt due to -Cdebuginfo, -Ccodegen-unit and etc change, which I think cannot be eliminated.

@pacak
Copy link
Owner

pacak commented Oct 19, 2022

Oh, that's because I don't forward --release flag yet.

I see... Current version uses --release by default. I tried to confirm what new version does using -vvv but that's not forwarded either :)

src/main.rs Outdated Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
@pacak
Copy link
Owner

pacak commented Oct 19, 2022

Anyway, let's try to get rust source annotation working and make sure it reuses the results as previously. Please let me know if I can help you with anything.

src/main.rs Outdated Show resolved Hide resolved
@oxalica
Copy link
Contributor Author

oxalica commented Oct 19, 2022

I implemented --bin and -example with hard-link tracking. It should locate the correct file even though there are multiple versions co-exist. clean should not be needed anymore.
I only tested it on Linux. Not sure if it works on other platforms. If not, we could add file content comparison as a same_file fallback.

Also I noticed that force_rebuild has no counterpart in cargo CLI. It seems can be replaced by a cargo clean. Could we remove this flag?

@pacak
Copy link
Owner

pacak commented Oct 19, 2022

I only tested it on Linux. Not sure if it works on other platforms. If not, we could add file content comparison as a same_file fallback.

Running Linux as well, but I have configured the CI to run Windows and it seems to be doing the right thing. I think the easiest way to test if it works on Windows too would be to adjust the CI to generate multiple files - probably by running it once, changing the file and running it again? I can look into that tomorrow.

Also I noticed that force_rebuild has no counterpart in cargo CLI. It seems can be replaced by a cargo clean. Could we remove this flag?

Removing is fine, if someone asks for it back I'll implement it using cargo clean

@figsoda
Copy link

figsoda commented Oct 20, 2022

escargot might be worth taking a look at

@pacak
Copy link
Owner

pacak commented Oct 20, 2022

I can look into that tomorrow.

Tried that and I'm not sure how to trick rustc into generating multiple files without making it overly complex system. I guess I'll check what samefile does on Windows, should be doing something sane I hope.

@oxalica Is there anything you are planning to add to this MR or should I proceed with testing/merging?

@oxalica
Copy link
Contributor Author

oxalica commented Oct 20, 2022

Tried that and I'm not sure how to trick rustc into generating multiple files without making it overly complex system.

Changing definitions of features and dependencies would change the hash in filenames I think.

@pacak
Copy link
Owner

pacak commented Oct 20, 2022

Changing definitions of features and dependencies would change the hash in filenames I think.

I totally forgot about features and went straight to dependencies :) Great idea, let me try that...

src/main.rs Show resolved Hide resolved
@pacak
Copy link
Owner

pacak commented Oct 20, 2022

escargot might be worth taking a look at

As far as I understand it is a bit more limited but user friendly version of spawning cargo commands directly. If that's the case I can try swapping it later, doesn't have to be in this MR.

@pacak
Copy link
Owner

pacak commented Oct 20, 2022

Okay, looks like currently it can't deal with multiple files on Windows either, but works on Linux so as long as it runs in Linux - it's a feature parity at least :)

@oxalica Can you rebase your branch on top of several-s-files to see if it works better?

@pacak
Copy link
Owner

pacak commented Oct 20, 2022

Can you rebase your branch on top of several-s-files to see if it works better?

Did it myself, it works.

@pacak pacak marked this pull request as ready for review October 21, 2022 12:15
@pacak pacak merged commit bed19f9 into pacak:master Oct 21, 2022
@oxalica
Copy link
Contributor Author

oxalica commented Oct 21, 2022

🤔 Oops, don't expect to be so eager. Just had a day break.

I think we should at least add a content comparison fallback for is_same_file here

Other code should be complete.

@pacak
Copy link
Owner

pacak commented Oct 21, 2022

Oops, don't expect to be so eager.

Mostly so I can keep pushing my changes on top of that, not going to publish it until everything is complete.

I think we should at least add a content comparison fallback for is_same_file here

It seems to be doing the right thing on Windows in CI. If you think it's important - feel free to throw one more pull request.

Other code should be complete.

Great 👍

@pacak
Copy link
Owner

pacak commented Oct 22, 2022

@oxalica Thank you very much for your contribution. New code is both faster to compile and it solved some issues with Windows in CI. I did some cleaning up and published it as 0.2.0 along with some breaking changes to command flags.

@oxalica oxalica deleted the no-libcargo branch October 24, 2022 00:36
@pacak
Copy link
Owner

pacak commented Nov 3, 2022

#82 - looks like there's something strange going on with rlib, I'm trying to go though artifacts as they are parsed and I don't see anything related to shadow-rs even though .s files are created. I wonder if you have any ideas, if not - I'll hack something to dig through the files directly in case of rlib...

Mystery solved, matches_artifact ignores it.

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.

[Feature] Possibly remove libcargo dependency?
3 participants