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

main_binary should reuse the caller's target #4

Closed
epage opened this issue May 28, 2018 · 4 comments
Closed

main_binary should reuse the caller's target #4

epage opened this issue May 28, 2018 · 4 comments
Labels
breaking-change bug Not as expected
Milestone

Comments

@epage
Copy link
Contributor

epage commented May 28, 2018

I suspect when running cargo test --target TRIPLET that main_binary will still use the default TRIPLE.

Downside: when the user is calling cargo test without a --target, assert_cmd passing --target will cause a one-time large compile time cost

See assert-rs/assert_cli#109

@epage
Copy link
Contributor Author

epage commented May 28, 2018

See assert-rs/assert_cli#102

@epage
Copy link
Contributor Author

epage commented Aug 2, 2018

From #43

@mssun

As described mesalock-linux/mesabox#41. I don't know why the first tests are always blocking for over 60 seconds. However, after the blocking issue, all testcases can be run without any problem.

test gnu::arch::test_x86_64 ... test gnu::arch::test_x86_64 has been running for over 60 seconds
test gnu::base32::test_decode ... test gnu::base32::test_decode has been running for over 60 seconds

This only happens in Travis: https://travis-ci.org/mesalock-linux/mesabox/jobs/411140601#L1305

I cannot reproduce this issue in my machine. Don't know why.

@epage

You are having the opposite problem of me with #4.

Your tests build with --target but assert_cmd doesn't. Even if they are the same target, cargo will do a unique build for it, blocking your tests. I'm guessing its two tests because they are running in parallel and one is being blocked on the other having the file lock.

This is a tricky situation. Ideally, we'd always pass along --target but for the default case, it'll slow people's tests down.

Options

@mssun

Yes, this is the problem. Testing with cargo test does not have this issue, but cargo test --target x86_64-unknown-linux-musl still has.

cargo test:

cargo test --target x86_64-unknown-linux-musl

This is a tricky situation. Ideally, we'd always pass along --target but for the default case, it'll slow people's tests down.

I'm not quite understand the tricky situation, and how it affects the cargo test --target x86_64-unknown-linux-musl case. Do you mean that no matter what target I pass to the cargo, assert_cmd will do a build for me (and without any target flag)?

Anyway, for the solution, I prefer to a esay/intuitive one like a feature flag to opt-in.

@epage
Copy link
Contributor Author

epage commented Aug 2, 2018

Do you mean that no matter what target I pass to the cargo, assert_cmd will do a build for me (and without any target flag)?

As of right now, yes.

  • If we start passing in --target, we hurt the performance of those who don't pass in --target.
  • But without it, we hurt your performance and reduce your platform coverage

@epage
Copy link
Contributor Author

epage commented Aug 3, 2018

I've started mapping out what we might due to the API to help

Please check out #44 and provide feedback

mssun pushed a commit to mesalock-linux/mesabox that referenced this issue Aug 5, 2018
The assert_cmd issue: assert-rs/assert_cmd#4
will make cargo to rebuild the binary and block the tests.
@epage epage added this to the 1.0 milestone Oct 6, 2018
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 in 403f612 Oct 7, 2018
epage added a commit to epage/assert_cmd that referenced this issue Jan 26, 2019
This is an experiemnt in a new direction in trying to resolve
- Cargo overhead (assert-rs#6)
- Compile times from mismatched `--target` (assert-rs#57)
- Suprising behavior if `--target <triplet>` isn't specified (assert-rs#4)

The new downsides this introduces
- No `main_binary` or `cargo_example`
- Can only work within integration tests

We're recommending the use of `escargot` in these cases which I think
might be reasonable because instead of making policy decisions for the
user, and no one ever being happy, the user can choose which policy they
want.  Plus, in more complex cases they should already be using
`escargot` so they can cache.

Fixes assert-rs#6
Fixes assert-rs#57
epage added a commit to epage/assert_cmd that referenced this issue Jan 26, 2019
This is an experiemnt in a new direction in trying to resolve
- Cargo overhead (assert-rs#6)
- Compile times from mismatched `--target` (assert-rs#57)
- Suprising behavior if `--target <triplet>` isn't specified (assert-rs#4)

The new downsides this introduces
- No `main_binary` or `cargo_example`
- Can only work within integration tests

We're recommending the use of `escargot` in these cases which I think
might be reasonable because instead of making policy decisions for the
user, and no one ever being happy, the user can choose which policy they
want.  Plus, in more complex cases they should already be using
`escargot` so they can cache.

Fixes assert-rs#6
Fixes assert-rs#57

BREAKING CHANGE: `main_binary` / `cargo_example` no longer exist.  Also,
successfully running `cargo_bin` has been restricted to integration
tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change bug Not as expected
Projects
None yet
Development

No branches or pull requests

1 participant