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

Add required-features to SerializedTarget #5902

Conversation

little-arhat
Copy link
Contributor

This will add it to cargo metadata output and will make it
possible to enable features needed to build specific target.

This will add it to `cargo metadata` output and will make it
possible to enable features needed to build specific target.
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@little-arhat
Copy link
Contributor Author

One of the use cases is flycheck/flycheck-rust#68. Let me know, if it's not an ideal solution!

@little-arhat
Copy link
Contributor Author

@alexcrichton hi Alex! Does it look like smth we can merge? Thanks!

@alexcrichton
Copy link
Member

Looks good to me, thanks! Could this also be renamed to required-features in the JSON?

@little-arhat
Copy link
Contributor Author

@alexcrichton Sure! I've pushed additional commit -- is it ok, or should I squash both commits together?
Field attribute is now longer than 80 chars, but it seems cargo codebase already has attributes like this. Happy to fix it though.

Thanks for you time!

@alexcrichton
Copy link
Member

@bors: r+

Looks good to me, thanks!

@bors
Copy link
Contributor

bors commented Aug 20, 2018

📌 Commit 349d264 has been approved by alexcrichton

@alexcrichton
Copy link
Member

@bors: retry

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Aug 21, 2018

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Aug 21, 2018

📌 Commit 349d264 has been approved by alexcrichton

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Aug 21, 2018

📌 Commit 9c38505 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Aug 21, 2018

⌛ Testing commit 9c38505 with merge 0dc7f69d0f1065b4a901b195acaa50795900d438...

@bors
Copy link
Contributor

bors commented Aug 21, 2018

💔 Test failed - status-appveyor

@little-arhat
Copy link
Contributor Author

failures:
---- build_script::switch_features_rerun stdout ----
running `C:\projects\cargo\target\debug\cargo.exe run -v --features=foo`
running `C:\projects\cargo\target\debug\cargo.exe run -v`
running `C:\projects\cargo\target\debug\cargo.exe run -v --features=foo`
thread 'build_script::switch_features_rerun' panicked at '
Expected: execs
    but: exited with exit code: 101
--- stdout
--- stderr
       Fresh foo v0.0.1 (file:///C:/projects/cargo/target/cit/t353/foo)
error: failed to link or copy `C:\projects\cargo\target\cit\t353\foo\target\debug\deps\foo-86673955dda3b155.exe` to `C:\projects\cargo\target\cit\t353\foo\target\debug\foo.exe`
Caused by:
  Access is denied. (os error 5)
', tests\testsuite\support\hamcrest.rs:13:9
note: Run with `RUST_BACKTRACE=1` for a backtrace.

hm, environment error?

@alexcrichton
Copy link
Member

@bors: retry

ah yeah, that's likely spurious

@ehuss
Copy link
Contributor

ehuss commented Aug 21, 2018

Looks like it would need the same treatment as changing_bin_features_caches_targets. It's unfortunate, and a little awkward. Personally I would prefer to add retry logic to cargo itself, since there is likely other tests that are vulnerable, and will likely be new ones in the future.

bors added a commit that referenced this pull request Aug 21, 2018
…etadata-output, r=alexcrichton

Add `required-features` to `SerializedTarget`

This will add it to `cargo metadata` output and will make it
possible to enable features needed to build specific target.
@bors
Copy link
Contributor

bors commented Aug 21, 2018

⌛ Testing commit 9c38505 with merge c0ec76f...

@bors
Copy link
Contributor

bors commented Aug 21, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing c0ec76f to master...

@little-arhat
Copy link
Contributor Author

@alexcrichton any idea when this feature will be available in nightly?

I've updated today and cargo is still from 0ec7281b9 2018-08-20.

@ehuss
Copy link
Contributor

ehuss commented Sep 3, 2018

@little-arhat update PR posted at rust-lang/rust#53935, once it gets through it should be in the next nightly.

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.

5 participants