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

Improved artifact matching and error messages #86

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

ZeroErrors
Copy link
Contributor

@ZeroErrors ZeroErrors commented Aug 1, 2020

Changes

  • Added more detail to error messages in Tool::rust_exec, Fixes: Error when llvm-tools-preview not installed could be clearer #79

  • Added target suggestion messages if no target was matched

  • Changed cargo build handling to collect only artifacts so log messages are output immediately

  • Changed all cases in BuildType::match to also check the target kind

  • Reorganized code to simplify logic flow by not relying on tool.needs_build() or tool to know if previous data was populated

  • Removed Context::from_flag as it was only used by Tool::Objdump and was only able to be called if no artifact was found, but since Tool::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

    • Removed serde, toml and related code as they had only been used by Context::from_flag
    • Inlined 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
fn main() {
    println!("Hello, World!");
}

(Confirmed build script is working with cargo clean then cargo build -vv)

Running cargo size and cargo strip with both v0.3.1 and the changes in this PR.

Tested suggestion messages by running cargo size on cargo-binutils

@rust-highfive
Copy link

r? @therealprof

(rust_highfive has picked a reviewer for you, use r? to override)

@therealprof
Copy link
Contributor

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

@ZeroErrors
Copy link
Contributor Author

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.

@therealprof
Copy link
Contributor

So I tried using cargo size on cargo-binutils itself:

error: Can only have one matching artifact but found several.
Specified build type: Any
Possible targets:
  cargo size --bench cargo-binutils
  cargo size --bin rust-nm
  cargo size --bin rust-objcopy
  cargo size --bin rust-strip
...

cargo size --bench cargo-binutils doesn't make sense and also doesn't work:

# cargo size --bench cargo-binutils
error: no bench target named `cargo-binutils`
error: Failed to run `cargo build` got exit code: 101
Use `cargo size -v ...` to display the full `cargo build` command

I'm also not happy with the output yet:

error: Can only have one matching artifact but found several.
Specified build type: Any
Possible targets:

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:

# cargo size --bin
error: The argument '--bin <NAME>' requires a value but none was supplied

USAGE:
    cargo-size <binary-name> --bin <NAME>

For more information try --help

@ZeroErrors
Copy link
Contributor Author

So I tried using cargo size on cargo-binutils itself:

error: Can only have one matching artifact but found several.
Specified build type: Any
Possible targets:
  cargo size --bench cargo-binutils
  cargo size --bin rust-nm
  cargo size --bin rust-objcopy
  cargo size --bin rust-strip
...

cargo size --bench cargo-binutils doesn't make sense and also doesn't work:

# cargo size --bench cargo-binutils
error: no bench target named `cargo-binutils`
error: Failed to run `cargo build` got exit code: 101
Use `cargo size -v ...` to display the full `cargo build` command

oh, woops. Yeah bad copy paste. Is supposed to be --lib
https://github.com/rust-embedded/cargo-binutils/pull/86/files#diff-b4aea3e418ccdb71239b96952d9cddb6R88

I'm also not happy with the output yet:

error: Can only have one matching artifact but found several.
Specified build type: Any
Possible targets:

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.

Yep will do a pass and change it to use target.

While experimenting with this I just noticed that the help generated by the CLI parser is off, but that can be addressed separately:

# cargo size --bin
error: The argument '--bin <NAME>' requires a value but none was supplied

USAGE:
    cargo-size <binary-name> --bin <NAME>

For more information try --help

hmm, yeah opened an issue for it.

@ZeroErrors
Copy link
Contributor Author

I'm also not happy with the output yet:

error: Can only have one matching artifact but found several.
Specified build type: Any
Possible targets:

Where does the build type come from and what does it mean?

The build type the argument provided, --lib, --bin <target>, etc.
It probably should get changed to only display if verbose output is enabled.

@therealprof
Copy link
Contributor

# cargo size
    Finished dev [unoptimized + debuginfo] target(s) in 0.09s
error: Can only have one matching target but found several.
Specified build type: Any
Possible targets:
  cargo size --lib cargo-binutils
  cargo size --bin cargo-size
  cargo size --bin cargo-objcopy
  cargo size --bin rust-objdump
  cargo size --bin rust-ar
  cargo size --bin rust-size
  cargo size --bin rust-ld
  cargo size --bin cargo-profdata
  cargo size --bin cargo-readobj
  cargo size --bin cargo-objdump
  cargo size --bin rust-lld
  cargo size --bin rust-readobj
  cargo size --bin cargo-nm
  cargo size --bin rust-objcopy
  cargo size --bin rust-nm
  cargo size --bin rust-profdata
  cargo size --bin rust-strip
  cargo size --bin cargo-strip
# cargo size --lib cargo-binutils
error: Found argument 'cargo-binutils' which wasn't expected, or isn't valid in this context

USAGE:
    cargo-size <binary-name> --lib

For more information try --help

😅

@therealprof
Copy link
Contributor

It also does surprising things on embedded projects now, e.g. when trying on the stm32f0xx-hal:

# cargo size --features="stm32f072,rt"
    Finished dev [unoptimized + debuginfo] target(s) in 0.04s
   text	   data	    bss	    dec	    hex	filename
   3306	      0	      0	   3306	    cea	stm32f0xx_hal-8764f603c6f243ae.15sok97r09wqx69d.rcgu.o (ex libstm32f0xx_hal.rlib)
     36	      0	      0	     36	     24	stm32f0xx_hal-8764f603c6f243ae.15vhf19n6ff33qjb.rcgu.o (ex libstm32f0xx_hal.rlib)
    214	      0	      0	    214	     d6	stm32f0xx_hal-8764f603c6f243ae.16g95b23l0dtk6k2.rcgu.o (ex libstm32f0xx_hal.rlib)
    144	      0	      0	    144	     90	stm32f0xx_hal-8764f603c6f243ae.16jw9k5j7mogltir.rcgu.o (ex libstm32f0xx_hal.rlib)
...

I certainly would not expect this to list the size of random anonymous compiler units...

@TDHolmes
Copy link

Any update on this? I just got a vague error message error: Could not determine the wanted artifact and presumably this PR would improve on that error message

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when llvm-tools-preview not installed could be clearer
4 participants