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

Introduce autoXXX keys for target auto-discovery #5335

Merged
merged 8 commits into from Apr 21, 2018

Conversation

Projects
None yet
6 participants
@dwijnand
Member

dwijnand commented Apr 10, 2018

In Rust 2015 absence of the configuration makes it default to not include auto-discovered targets (i.e false), with a warnings message.

In Rust 2018 absence makes it default to include auto-discovered targets (i.e true).

Fixes #5330


original

Fixes #5330

submitted for early review. feedback very welcome, I'm happy to iterate (and learn).

in particular I require assistance with the borrowing of warnings in clean_benches.

@rust-highfive

This comment has been minimized.

rust-highfive commented Apr 10, 2018

r? @alexcrichton

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

@dwijnand dwijnand changed the title from Introduce {target}.autodiscovery to opt out after 2018 Fixes #5330 to Introduce {target}.autodiscovery to opt out after 2018 Apr 10, 2018

@dwijnand

This comment has been minimized.

Member

dwijnand commented Apr 10, 2018

oh, also I'm only testing examples at the moment. is that sufficient? if not how should I further test this?

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Apr 10, 2018

Awesome, thanks for this! I think yeah we'll probably want tests for at least one type of target, but it's fine to not need tests for all of them.

I"m nto sure that autodiscovery is in the right location though as it's sort of a project-level distinction rather than a per-target distinction. Perhaps we could have keys like auto-examples = false inside the [package] section?

As we discussed on the other PR as well I think we'll want to start issuing warnings in the 2015 epoch if we'd like to message this out as part of the 2018 release

@dwijnand

This comment has been minimized.

Member

dwijnand commented Apr 10, 2018

I think yeah we'll probably want tests for at least one type of target, but it's fine to not need tests for all of them.

Do you mean you want one more? I've already got one test for the examples target.

I"m nto sure that autodiscovery is in the right location though as it's sort of a project-level distinction rather than a per-target distinction. Perhaps we could have keys like auto-examples = false inside the [package] section?

I'll give that a shot, see how it sits.

As we discussed on in other PR as well I think we'll want to start issuing warnings in the 2015 epoch if we'd like to message this out as part of the 2018 release

👍

Any tips to resolve my borrowchecker woes?

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Apr 10, 2018

Oh oops sorry I meant at least two types of targets!

I'm on my phone but I'll check back for the borrow checker when I'm back at a computer

edition,
// FIXME: cannot borrow `*warnings` as mutable because previous closure requires unique access
// warnings,
&mut vec![],

This comment has been minimized.

@alexcrichton

alexcrichton Apr 10, 2018

Member

Could a temporary vector be allocated here and then appended to warnings outside the scope of the closure above?

This comment has been minimized.

@dwijnand

dwijnand Apr 10, 2018

Member

thanks! ideally I'd want the other way (e.g legacy_path_warnings), but I can't get it to work.

@bors

This comment has been minimized.

Contributor

bors commented Apr 12, 2018

☔️ The latest upstream changes (presumably #5348) made this pull request unmergeable. Please resolve the merge conflicts.

@dwijnand

This comment has been minimized.

Member

dwijnand commented Apr 12, 2018

Please resolve the merge conflicts.

by merging or rebasing?

@matklad

This comment has been minimized.

Member

matklad commented Apr 12, 2018

@dwijnand I personally slightly prefer rebasing, but in general I don't think we care a lot about history :)

@bors

This comment has been minimized.

Contributor

bors commented Apr 16, 2018

☔️ The latest upstream changes (presumably #5360) made this pull request unmergeable. Please resolve the merge conflicts.

@dwijnand

This comment has been minimized.

Member

dwijnand commented Apr 16, 2018

this is ready for review.

I believe it now implements the desired behaviour discussed in #5330 (and here).

feedback always welcome :)

@dwijnand dwijnand changed the title from Introduce {target}.autodiscovery to opt out after 2018 to Introduce target auto-discovery options Apr 16, 2018

@alexcrichton

This is looking fantastic @dwijnand, thanks so much!

To summary to make sure I understand, we've got a bunch new keys under [package] like autobins = true which in 2018 default to true and in 2015 default to true if no [[bin]] is specified or false otherwise. In 2015 if autobins = false implicitly and we would have otherwise inferred a new target we issue a warning. Does that sound right?

Could you add some tests for autoXXX = false and the warning being disabled? Also, could the documentation in src/doc be updated with the new manifest keys?

"No auto-discovered {target_kind_human} targets included \
because at least one explicit [[{target_kind}]] section was added to Cargo.toml. \
Define the auto-discovery option for targets of this type, \
and beware that in Rust 2018 edition the default will flip to `true`.",

This comment has been minimized.

@alexcrichton

alexcrichton Apr 17, 2018

Member

Could this perhaps be reworded to:

An explicit [[{section}]] section is specified in Cargo.toml which currently disables Cargo from automatically inferring what other [[{section}]] targets would be. Disabling this inference behavior will change in the Rust 2018 edition and the following files will be included as a [[{section}]] target:

  • src/bin/foo.rs
  • src/bin/baz.rs

This is likely to break cargo build or cargo test as these files may not be ready to be compiled as a {{section}} target today. You can future-proof yourself and disable this warning by adding auto{{section}} = false to your [package] section. You may also move the files to a location where Cargo would not automatically infer them to be a target, such as in subfolders.

For more information on this warning you can consult https://github.com/rust-lang/cargo/issues/NUMBER

@@ -444,6 +444,7 @@ fn bench_with_lib_dep() {
name = "foo"
version = "0.0.1"
authors = []
autobins = true

This comment has been minimized.

@alexcrichton

alexcrichton Apr 17, 2018

Member

Where autoXXX = true was added to the test suite could the files be reorganized a bit to not trigger the warning or need the key?

This comment has been minimized.

@dwijnand

dwijnand Apr 18, 2018

Member

let me have a look.

"#,
);
if cargotest::is_nightly() {

This comment has been minimized.

@alexcrichton

alexcrichton Apr 17, 2018

Member

This I believe can be avoided with masquerade_as_nightly_cargo() for Cargo, and there should be one #[test] function per test (so split this function into two).

This test may still need to be at the top of the function though because --edition doesn't exist on stable rustc, but it'll just be a prelude to all these tests.

This comment has been minimized.

@dwijnand

dwijnand Apr 18, 2018

Member

will do

This comment has been minimized.

@dwijnand

dwijnand Apr 18, 2018

Member

actually can you explain again, please? the only part I'm clear on is splitting the test.

what I'm testing here is:

  • in rust nightly + cargo edition feature + edition 2018 => the inferred examples/a.rs isn't dropped in the presence of a declared "do_magic" example
  • in rust stable (and therefore edition 2015) => the presence of a declared "do_magic" example, and the lack of an autoXXX, causes the inferred examples/a.rs to be dropped, therefore causing a warning and the cargo run --example a to fail

This comment has been minimized.

@alexcrichton

alexcrichton Apr 18, 2018

Member

Oh sure yeah, so the way I see this test case is that there's actually two test cases internally and it's dynamically dispatched depending on rustc. I'd prefer to to split this out into two separate test cases where they may just be skipped entirely for nightly Rust

@dwijnand

This comment has been minimized.

Member

dwijnand commented Apr 18, 2018

thanks for the review @alexcrichton.

I've been working on this offline to get the test suite to pass - the commits I just pushed should CI go green now. I'm still stumbling my way to compiling and test-passing code, so please let me know if you spot any eye-soars or the like.

To summary to make sure I understand, we've got a bunch new keys under [package] like autobins = true which in 2018 default to true and in 2015 default to true if no [[bin]] is specified or false otherwise. In 2015 if autobins = false implicitly and we would have otherwise inferred a new target we issue a warning. Does that sound right?

it's hard to reason about this in terms of "default" because there are so many input variables. my attempt to summarise would be:

  • in 2015 you get a warning if you're dropping inferred targets
  • in 2018 you don't drop them, and therefore don't get a warning
  • with autoXXX you can specify whether to drop or not inferred targets (with false and true, respectively)
@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Apr 18, 2018

Ok sounds great!

dwijnand added some commits Apr 10, 2018

Move edition earlier in TomlManifest::to_real_manifest
and fix a typo in the error message
@dwijnand

This comment has been minimized.

Member

dwijnand commented Apr 20, 2018

hey @alexcrichton, I think I've covered everything.

sorry for the force push but I think you'll find it easier to review this way (instead of the crazy git history tale of my progress to the end zone).

please let me know, happy to iterate with your feedback.

@dwijnand dwijnand changed the title from Introduce target auto-discovery options to Introduce autoXXX keys for target auto-discovery Apr 20, 2018

Introduce autoXXX keys for target auto-discovery
In Rust 2015 absence of the configuration makes it default to not
include auto-discovered targets (i.e false), with a warnings message.

In Rust 2018 absence makes it default to include auto-discovered
targets (i.e true).

Fixes #5330
@dwijnand

This comment has been minimized.

Member

dwijnand commented Apr 20, 2018

I thought I had understood that [/] is precisely to handle unix/windows paths, and yet...

  2 - |* "[..]foo[/]benches[/]bench_basic.rs"|
    + |* "C:\\projects\\cargo\\target\\cit\\t114\\foo\\benches\\bench_basic.rs"|
  2 - |* "[..]foo[/]examples[/]a.rs"|
    + |* "C:\\projects\\cargo\\target\\cit\\t952\\foo\\examples\\a.rs"|

what am I missing?

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Apr 20, 2018

Hm, weird! I would have thought those would be working assertions as well... In any case it's fine to just use [..]bench_basic.rs as the assertion and not worry too much about whatever bug may be happening here

@alexcrichton

Thanks again so much for working on this! Just a few small remaining nits

"\
An explicit [[{section}]] section is specified in Cargo.toml which currently disables Cargo from \
automatically inferring other {target_kind_human} targets. This inference behavior will change in \
the Rust 2018 edition and the following files will be included as a {target_kind_human} target:

This comment has been minimized.

@alexcrichton

alexcrichton Apr 20, 2018

Member

When using string literals for warnings in Cargo I've historically preferred to have manual line wrapping at 80 characters at that tends to be a good default at least for most terminals to display well. That'd just involve wrapping here along with removing the \ at the end of the lines

{rem_targets_str}
This is likely to break cargo build or cargo test as these files may not be ready to be compiled \
as a {target_kind_human} target today. You can future-proof yourself and disable this warning by \
adding {autodiscover_flag_name} = false to your [package] section. You may also move the files to \

This comment has been minimized.

@alexcrichton

alexcrichton Apr 20, 2018

Member

Could the autofoo = false be surrounded by backticks here?

let mut rem_targets_str = String::new();
for t in rem_targets.iter() {
if let Some(p) = t.path.clone() {
rem_targets_str.push_str(&format!("* {:?}\n", p.0))

This comment has been minimized.

@alexcrichton

alexcrichton Apr 20, 2018

Member

I'd recommend using p.0.display() here instead fo {:?} to avoid the extraneous quotes

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Apr 20, 2018

I've updated #5330 with the current state of this proposal as well. @rust-lang/cargo now would be a good time to review that issue's description and make sure you're ok with it!

dwijnand added some commits Apr 20, 2018

@dwijnand

This comment has been minimized.

Member

dwijnand commented Apr 20, 2018

that should be everything. thanks, the docs in #5330 look great.

do you think the docs for the manifest keys in reference guide are sufficient?

let targets = {
let mut legacy_bench_path = |bench: &TomlTarget| {
let legacy_path = package_root.join("src").join("bench.rs");

This comment has been minimized.

@matklad

matklad Apr 21, 2018

Member

A nice thing to do in the next PR would be to add if edition != Edition::Edition2015 { return None; } over here!

This comment has been minimized.

@dwijnand

dwijnand Apr 21, 2018

Member

ah, this must be one of the instances of "backwards compatibility baggage" you originally referred to in #5330.

sure!

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Apr 21, 2018

@dwijnand ah yeah the docs are good for now, we can always expand later if necessary!

@bors: r+

@bors

This comment has been minimized.

Contributor

bors commented Apr 21, 2018

📌 Commit ab5ac28 has been approved by alexcrichton

@bors

This comment has been minimized.

Contributor

bors commented Apr 21, 2018

⌛️ Testing commit ab5ac28 with merge fd3be77...

bors added a commit that referenced this pull request Apr 21, 2018

Auto merge of #5335 - dwijnand:target-autodiscovery, r=alexcrichton
Introduce autoXXX keys for target auto-discovery

In Rust 2015 absence of the configuration makes it default to not
include auto-discovered targets (i.e false), with a warnings message.

In Rust 2018 absence makes it default to include auto-discovered
targets (i.e true).

Fixes #5330

---

_original_

Fixes #5330

submitted for early review. feedback very welcome, I'm happy to iterate (and learn).

in particular I require assistance with the borrowing of `warnings` in `clean_benches`.
@bors

This comment has been minimized.

Contributor

bors commented Apr 21, 2018

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

@bors bors merged commit ab5ac28 into rust-lang:master Apr 21, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@dwijnand dwijnand deleted the dwijnand:target-autodiscovery branch Apr 21, 2018

@ehuss

This comment has been minimized.

Contributor

ehuss commented Apr 21, 2018

Just FYI, building cargo with itself now displays this message. 😄

warning: An explicit [[bin]] section is specified in Cargo.toml which currently
disables Cargo from automatically inferring other binary targets.
This inference behavior will change in the Rust 2018 edition and the following
files will be included as a binary target:

* /Users/eric/Proj/rust/cargo/src/bin/cli.rs
* /Users/eric/Proj/rust/cargo/src/bin/command_prelude.rs

bors added a commit that referenced this pull request Apr 24, 2018

Auto merge of #5398 - dwijnand:drop-legacy-paths, r=matklad
Drop legacy path support under Rust edition 2018 (or later)

builds on #5335

submitted for early feedback: wdyt @matklad? is this what you had in mind? what should change? what should be added? how should we test this? is the current (2015) messaging enough to drop it in 2018?

r? @matklad

<!--{"baseBranch":"rust-lang:cargo:target-autodiscovery"}-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment