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

Try to determine if --target was passed to Cargo #25

Merged
merged 3 commits into from
Aug 21, 2020
Merged

Try to determine if --target was passed to Cargo #25

merged 3 commits into from
Aug 21, 2020

Conversation

adamreichold
Copy link
Contributor

This checks the output directory for the $CARGO_TARGET_DIR/$TARGET pattern making use of MAIN_SEPARATOR to try to handle Windows. It will also fail if either the path or that pattern are not valid UFT-8, but it will then fall back to the current behavior. (So this basically tries to implement #10 (comment).)

Copy link
Owner

@cuviper cuviper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks pretty reasonable, just a few comments below.

Out of curiosity, does this solve any real-world problems for you? If so, can you describe that?

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@adamreichold
Copy link
Contributor Author

Out of curiosity, does this solve any real-world problems for you? If so, can you describe that?

Yes, we just changed the atomic crate to use autocfg to probe for the availability of the Atomic{U,I}{8,16,32,64,128} in core. For amd64, one needs to set RUSTFLAGS=-Ctarget-feature=+cx16 -Zcrate-attr=feature(integer_atomics) to get access to the Atomic{U,I}128 types which is currently not possible as this is not a cross build scenario w.r.t. the current definition even if --target is given explicitly and hence we had to resort to build for AArch64 to test this.

This checks the output directory for the $CARGO_TARGET_DIR/$TARGET pattern
making use of MAIN_SEPARATOR to try to handle Windows. It will also fail if
either the path or that pattern are not valid UFT-8, but it will then fall back
to the current behavior.
@adamreichold
Copy link
Contributor Author

@cuviper Just a friendly ping if there is anything that still needs to be changed here?

@cuviper
Copy link
Owner

cuviper commented Aug 18, 2020

This looks good, although I wish we had a test. I'm not sure how we would do that though -- maybe a workspace crate that can use a real build script, driving with and without --target in the CI script?

I hope you have at least verified that it works in your scenario?

@adamreichold
Copy link
Contributor Author

I hope you have at least verified that it works in your scenario?

Yes, I have checked that this allows me to pass RUSTFLAGS into the probes using cargo --target ${HOST} but ...

This looks good, although I wish we had a test. I'm not sure how we would do that though -- maybe a workspace crate that can use a real build script, driving with and without --target in the CI script?

... I will add a unit test by factoring out the detection logic. I think the direct feedback during development is more important than the accuracy of testing an actual Cargo invocation.

@cuviper
Copy link
Owner

cuviper commented Aug 18, 2020

I think the direct feedback during development is more important than the accuracy of testing an actual Cargo invocation.

Fair -- I probably lean too often on black-box testing.

src/tests.rs Outdated Show resolved Hide resolved
src/tests.rs Outdated Show resolved Hide resolved
@cuviper
Copy link
Owner

cuviper commented Aug 21, 2020

Thanks! I'll go ahead and publish 1.0.1 from here.

bors r+

@bors bors bot merged commit d2c6034 into cuviper:master Aug 21, 2020
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.

None yet

2 participants