Skip to content

RFC Greater customization of cargo #44

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

Closed
epage opened this issue Aug 3, 2018 · 8 comments
Closed

RFC Greater customization of cargo #44

epage opened this issue Aug 3, 2018 · 8 comments
Labels
breaking-change enhancement Improve the expected
Milestone

Comments

@epage
Copy link
Contributor

epage commented Aug 3, 2018

Use cases

Considerations

  • The primary use case of assert_cmd is testing Rust binaries, so we should help those users
  • An entire cargo API is orthogonal to assert_cmd and would be best to be stablized outside of assert_cmd

Proposal

  • Mirror the concept of cargo run in escargot but via CargoBuild
    • CargoBuild would have a .run() method that would return a CargoRun
    • CargoRun would have .path() and .command() methods.
  • Still provide CommandCargoExt but point people to escargot for any custom behavior
  • Deprecate the cargo path functions and instead point people to escargot for those use cases.
    • Create an issue for them to be deleted as part of the 1.0 milestone

At this point, CommandCargoExt is primarily serving a low-effort path for a "getting started" user while serving as an example of how to do more complex things (via code) and pointing to more complex APIs (via docs)

Open Issues

@epage
Copy link
Contributor Author

epage commented Aug 5, 2018

@quininer and @mssun, I've released escargot v0.3.0 with CargoRun support (see docs).

Could you give that a spin to see what you thoughts are? If it works for you, I'll be closing this issue by

  • Removing the _path functions from the API (and by extension, removing the cargo mod from the API)
  • Updating the docs to point to escargot for advanced use cases (custom builds, caching, etc).

We still need to decide whether the easy-path API should use current_target or not.

@quininer
Copy link

quininer commented Aug 5, 2018

I am satisfied with this API, but CargoRun does not seem to work for --example. I got "No binaries in crate".

@mssun
Copy link
Contributor

mssun commented Aug 5, 2018

Thanks @epage, I'm pretty satisfied with the API.

Now I can use

 lazy_static! {
     pub static ref BIN_PATH: PathBuf = escargot::CargoBuild::new()
         .bin("binary_name")
         .current_release()
         .current_target()
         .run()
         .unwrap()
         .path()
         .to_path_buf();
 }

to get the path of binary which compiled with --target xxx.

This works well when test with cargo test --target xxx.

However, when I don't use --target xxx to test (e.g., test the default target: x86_64-unknown-linux-gnu), escargot will still build the binary again and put it into target/x86_64-unknown-linux-gnu/debug instead of target/debug. Seems that escargot cannot detect the existence of the --target flag.

@epage
Copy link
Contributor Author

epage commented Aug 6, 2018

@quininer

I am satisfied with this API, but CargoRun does not seem to work for --example. I got "No binaries in crate".

I know exactly whats wrong and will hopefully get a chance to fix it soon. In the mean time, could you open an issue for it on escargot?

@mssun

How come you are caching the path rather than caching the CargoRun and then using BIN.command() to access all the assert_cmd features

However, when I don't use --target xxx to test (e.g., test the default target: x86_64-unknown-linux-gnu), escargot will still build the binary again and put it into target/x86_64-unknown-linux-gnu/debug instead of target/debug. Seems that escargot cannot detect the existence of the --target flag.

It cannot and that is the crux of #4.

@mssun
Copy link
Contributor

mssun commented Aug 6, 2018

It cannot and that is the crux of #4.

Yes, I understand. Thank you. This requires me to run cargo test with --target in any case. But, seems that there is no solution for this.

epage added a commit to epage/assert_cmd that referenced this issue Aug 9, 2018
@epage epage closed this as completed in #46 Aug 9, 2018
@epage epage reopened this Aug 9, 2018
@epage
Copy link
Contributor Author

epage commented Aug 9, 2018

Remaining work: figure out target

@mssun
Copy link
Contributor

mssun commented Sep 4, 2018

Specifying features (see #34)

Can I specify the features now? The use case is that we want to test some features which are not default.

@epage
Copy link
Contributor Author

epage commented Sep 4, 2018

Yes, though it will require adding a dependency on escargot and callling into that, rather than using CommandCargoExt.

See the implementation of CommandCargoExt for how you might do this:: https://github.com/assert-rs/assert_cmd/blob/master/src/cargo.rs#L137

Drat, it doesn't look like I've added a feature argument to escargot. I've created crate-ci/escargot#10. Feel free to implement it or use the suggested workaround. Eventually I'm going to need it anyways but I'm assuming your needs are more pressing.

epage added a commit to epage/assert_cmd that referenced this issue Oct 6, 2018
Part of assert-rs#44.

BREAKING CHANGE: Deprecated cargo path functions removed.
epage added a commit to epage/assert_cmd that referenced this issue Oct 6, 2018
I decided to go with this because I feel like correctness trumps
performance.

For those not specifying `--target`, the performance will surprise them.

For those specifying `--target`, the lack of correctness will surprise
them but is less easily detected.

Fixes assert-rs#4, assert-rs#44

BREAKING CHANGE: In this case, performance is a breaking change because
this might cause CI's to time out.
epage added a commit to epage/assert_cmd that referenced this issue Oct 6, 2018
Part of assert-rs#44.

BREAKING CHANGE: Deprecated cargo path functions removed.
epage added a commit to epage/assert_cmd that referenced this issue Oct 6, 2018
I decided to go with this because I feel like correctness trumps
performance.

For those not specifying `--target`, the performance will surprise them.

For those specifying `--target`, the lack of correctness will surprise
them but is less easily detected.

Fixes assert-rs#4, assert-rs#44

BREAKING CHANGE: In this case, performance is a breaking change because
this might cause CI's to time out.
@epage epage closed this as completed Oct 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change enhancement Improve the expected
Projects
None yet
Development

No branches or pull requests

3 participants