-
Notifications
You must be signed in to change notification settings - Fork 47
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
Improved artifact matching and error messages #86
base: master
Are you sure you want to change the base?
Conversation
r? @therealprof (rust_highfive has picked a reviewer for you, use r? to override) |
Wow, that is a very comprehensive PR, thanks. I think I should be improving CI before we land this. I have a feeling the formatting is off in parts, did you run rustfmt? Also it would be great to have some automatic regression tests, better than https://github.com/rust-embedded/cargo-binutils/blob/master/ci/script.sh |
Yeah my IDE is setup to run rustfmt on file save and it runs clippy for linting. Agree with the testing. I was thinking it could be a good idea to have a sub-directory with a few different test crates with different structures to check that bin, lib, examples, etc. and combinations of as well as workspaces and such still work correctly. |
So I tried using
I'm also not happy with the output yet:
Where does the build type come from and what does it mean? And why are we talking about artifacts and targets, we should probably unify the nomenclature here by just talking about targets. While experimenting with this I just noticed that the help generated by the CLI parser is off, but that can be addressed separately:
|
oh, woops. Yeah bad copy paste. Is supposed to be
Yep will do a pass and change it to use target.
hmm, yeah opened an issue for it. |
The build type the argument provided, |
😅 |
It also does surprising things on embedded projects now, e.g. when trying on the
I certainly would not expect this to list the size of random anonymous compiler units... |
Any update on this? I just got a vague error message |
Changes
Added more detail to error messages in
Tool::rust_exec
, Fixes: Error when llvm-tools-preview not installed could be clearer #79Added target suggestion messages if no target was matched
Changed
cargo build
handling to collect only artifacts so log messages are output immediatelyChanged all cases in
BuildType::match
to also check the targetkind
Reorganized code to simplify logic flow by not relying on
tool.needs_build()
ortool
to know if previous data was populatedRemoved
Context::from_flag
as it was only used byTool::Objdump
and was only able to be called if no artifact was found, but sinceTool::Objdump
requires a build this is not possible.This just seems to be left over code from the changes made for: Use main binary if nothing else is specified #39
serde
,toml
and related code as they had only been used byContext::from_flag
Context::from_artifact
Testing
Tested with both
cargo new --bin test_bin_crate
cargo new --lib test_lib_crate
Adding a build script
build.rs
(Confirmed build script is working with
cargo clean
thencargo build -vv
)Running
cargo size
andcargo strip
with both v0.3.1 and the changes in this PR.Tested suggestion messages by running
cargo size
oncargo-binutils