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

Make explicit opt-out of target discovery neccessary for Rust 2018 #5330

Closed
matklad opened this issue Apr 9, 2018 · 7 comments · Fixed by #5335
Closed

Make explicit opt-out of target discovery neccessary for Rust 2018 #5330

matklad opened this issue Apr 9, 2018 · 7 comments · Fixed by #5335
Labels
E-hard Experience: Hard

Comments

@matklad
Copy link
Member

matklad commented Apr 9, 2018

Currently, Cargo automatically discovers tests, examples and binaries form the files on the file system. One can also add an explicit [[example]] section to Cargo.toml, to add non-discoverable examples, or to tweak an existing example (for example, by specifying required features).

However, adding explicit [[target]] disables automatic discovery for other targets of similar type, and this behavior is highly surprising.

Current Proposal

New keys will be available under the [package] section of Cargo.toml:

[package]
# ... 

autobins = true
autoexamples = true
autotests = true
autobenches = true

The value of each of these keys indicates whether or not Cargo's automatic inference logic is in effect for discovering targets. Turning these keys to false will completely disable inference and Cargo will not attempt to find more targets. For example setting autobins = false will disable Cargo probing src/bin looking for binaries.

If these keys are turned to true then Cargo will probe the standard set of directories for targets. Any explicitly named targets in the manifest will be merged with the automatically found targets. For example if autotests = true is enabled and you've got tests/foo.rs and tests/bar.rs, you could disable the test harness of tests/foo.rs with:

[[test]]
name = "foo"
harness = false

and you'll still be able to use cargo test --test bar as Cargo will automatically find the tests/bar.rs test.

In the 2015 edition (today in Cargo) these keys are set with the following rules:

  • If an explicit target is specified, the key is set to false. For example writing [[bin]] will set autobins = false automatically.
  • If no explicit targets are specified, the key is set to true.

In the 2018 edition these keys will be unconditionally set to true by default.

Impact on Users Today

Unconditionally turning these keys to true is a breaking change as it can cause files that weren't previously compiled as a test, for example, to get compiled as a test. As a result projects will need to reorganize their files or otherwise set autotests = true, for example.

If you receive a command-line warning though and don't know what to do with that, please leave a comment here!

Original Proposal

We would like to change this behavior eventually and for that in Rust 2018 we should:

  • add a flag autodiscover for each target type, with values true and false (precise flag name and syntax are subject to bike shedding).
  • in Rust 2018, warn if autodiscover is not specified, at least a single [[target]] is listed, and some other target would be discovered if autodiscover was set to true.
  • in the edition after 2018, flip autodiscover default to true.

The relevant code is here: https://github.com/rust-lang/cargo/blob/d8b20310217e5e2bc515c13111ab9a7789709e42/src/cargo/util/toml/targets.rs. Note that the logic there is pretty subtle, because we already carry a significant backwards compatibility baggage :(

It might be a good idea to flip some of those warnings to errors in 2018 as well!

@dwijnand
Copy link
Member

coming back to this, from @alexcrichton's comment in my #5335 proposal:

I'm not 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

(the other PR is #5333)

is the desired behaviour, under the condition that you specified a target, like [[example]], for non-discoverability or configuration purposes, that:

  • in Rust 2015 a warning is issued that discoverable targets are not being included, but will be in Rust 2018
  • in Rust 2018 discoverable target are always included

does that sound right?

@alexcrichton
Copy link
Member

@dwijnand I believe so yeah! We'll still want some way to say "I've exhaustively listed all targets here, please do not continue to infer", but other than that your description sounds accurate.

@dwijnand
Copy link
Member

part of the reason for my brainstorming is I was wondering if we could avoid the repetition of a key for each target in [package] by having something like no-auto = ["examples"]. But then I couldn't tell if it was more important to explicitly opt-out or opt-in of auto-discovery, so I'll just go with your initial idea and take it from there.

@alexcrichton
Copy link
Member

Oh I'd be fine as well with something like no-auto = ["examples"]!

@dwijnand
Copy link
Member

so that would work well for your "I've exhaustively listed all targets here, please do not continue to infer".

but I wonder: what do Rust 2015 users do to silence the warning? if they don't want to infer they too can add their target no-auto. but what if they want all targets included? should the solution there be defining no-auto = []? seems a bit subtle.

@alexcrichton
Copy link
Member

@dwijnand oh for 2015 users I think it's the same as 2018, you'd specify no-auto = [...]

In general we can't really do much for the 2018 edition that we couldn't already do through slow deprecation, so it's mostly just serving as a good banner time to get things done by and send out messaging.

bors added a commit that referenced this issue Apr 21, 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

---

_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`.
SimonSapin added a commit to servo/servo that referenced this issue May 3, 2018
SimonSapin added a commit to servo/servo that referenced this issue May 4, 2018
iliana added a commit to wobscale/pepbut that referenced this issue May 16, 2018
Moggers pushed a commit to Moggers/servo that referenced this issue Jun 13, 2018
mssun pushed a commit to mesalock-linux/mesabox that referenced this issue Jul 18, 2018
Since we explicit add a test section to specify the `tests.rs` file,
there is no need for auto tests inferece. This commit is to disable the annoying
warning:

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

* util.rs
* macros.rs

This is likely to break cargo build or cargo test as these files may not be
ready to be compiled as a test target today. You can future-proof yourself
and disable this warning by adding `autotests = 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
rust-lang/cargo#5330
Nicoretti pushed a commit to Nicoretti/xxd-rs that referenced this issue Oct 5, 2018
`cargo build` warns:

    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/kivikakk/Code/Nicoretti/xxd-rs/src/bin/main.rs
    * /Users/kivikakk/Code/Nicoretti/xxd-rs/src/bin/cli.rs

    This is likely to break cargo build or cargo test as these files may not be
    ready to be compiled as a binary target today. You can future-proof yourself
    and disable this warning by adding `autobins = 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
    rust-lang/cargo#5330
@elichai
Copy link

elichai commented Oct 28, 2019

If you receive a command-line warning though and don't know what to do with that, please leave a comment here!

I'm getting this warning. i'm trying to enable a feature in the crate for one of my examples. (currently just added autoexamples = false)

djc pushed a commit to servo/rust-url that referenced this issue Aug 25, 2020
epage added a commit to epage/cargo that referenced this issue Sep 24, 2024
PR rust-lang#5335 added `autobins`, etc for rust-lang#5330.  Nowhere in there is
discussion of `autolib`.

Cargo script disables support for additional build-targets by disabling
discovery.
Except we don't have a way to disable discovery of `autolib`, leading to rust-lang#14476.
By adding `autolib`, we can continue in that direction.

This also allows us to bypass inferring of libs on published packages,
like all other build-targets which were handled in rust-lang#13849.

As this seems fairly low controversy, this insta-stabilizes the field.
In prior versions of Cargo, users will get an "unused manifest key"
warning.
For packags where this is set by `cargo publish`, the warning will be suppressed and things will work as normal.
For `cargo vendor`, the same except there will be some churn in the
vendored source as this field will now be set.
For local development, it should be rare to set `autolib` so the lack of
error by discovering a file when this is set shouldn't be a problem.

Fixes rust-lang#14476
epage added a commit to epage/cargo that referenced this issue Sep 24, 2024
PR rust-lang#5335 added `autobins`, etc for rust-lang#5330.  Nowhere in there is
discussion of `autolib`.

Cargo script disables support for additional build-targets by disabling
discovery.
Except we don't have a way to disable discovery of `autolib`, leading to rust-lang#14476.
By adding `autolib`, we can continue in that direction.

This also allows us to bypass inferring of libs on published packages,
like all other build-targets which were handled in rust-lang#13849.

As this seems fairly low controversy, this insta-stabilizes the field.
In prior versions of Cargo, users will get an "unused manifest key"
warning.
For packags where this is set by `cargo publish`, the warning will be suppressed and things will work as normal.
For `cargo vendor`, the same except there will be some churn in the
vendored source as this field will now be set.
For local development, it should be rare to set `autolib` so the lack of
error by discovering a file when this is set shouldn't be a problem.

Fixes rust-lang#14476
epage added a commit to epage/cargo that referenced this issue Sep 25, 2024
PR rust-lang#5335 added `autobins`, etc for rust-lang#5330.  Nowhere in there is
discussion of `autolib`.

Cargo script disables support for additional build-targets by disabling
discovery.
Except we don't have a way to disable discovery of `autolib`, leading to rust-lang#14476.
By adding `autolib`, we can continue in that direction.

This also allows us to bypass inferring of libs on published packages,
like all other build-targets which were handled in rust-lang#13849.

As this seems fairly low controversy, this insta-stabilizes the field.
In prior versions of Cargo, users will get an "unused manifest key"
warning.
For packags where this is set by `cargo publish`, the warning will be suppressed and things will work as normal.
For `cargo vendor`, the same except there will be some churn in the
vendored source as this field will now be set.
For local development, it should be rare to set `autolib` so the lack of
error by discovering a file when this is set shouldn't be a problem.

Fixes rust-lang#14476
epage added a commit to epage/cargo that referenced this issue Sep 26, 2024
PR rust-lang#5335 added `autobins`, etc for rust-lang#5330.  Nowhere in there is
discussion of `autolib`.

Cargo script disables support for additional build-targets by disabling
discovery.
Except we don't have a way to disable discovery of `autolib`, leading to rust-lang#14476.
By adding `autolib`, we can continue in that direction.

This also allows us to bypass inferring of libs on published packages,
like all other build-targets which were handled in rust-lang#13849.

As this seems fairly low controversy, this insta-stabilizes the field.
In prior versions of Cargo, users will get an "unused manifest key"
warning.
For packags where this is set by `cargo publish`, the warning will be suppressed and things will work as normal.
For `cargo vendor`, the same except there will be some churn in the
vendored source as this field will now be set.
For local development, it should be rare to set `autolib` so the lack of
error by discovering a file when this is set shouldn't be a problem.

Fixes rust-lang#14476
epage added a commit to epage/cargo that referenced this issue Sep 26, 2024
PR rust-lang#5335 added `autobins`, etc for rust-lang#5330.  Nowhere in there is
discussion of `autolib`.

Cargo script disables support for additional build-targets by disabling
discovery.
Except we don't have a way to disable discovery of `autolib`, leading to rust-lang#14476.
By adding `autolib`, we can continue in that direction.

This also allows us to bypass inferring of libs on published packages,
like all other build-targets which were handled in rust-lang#13849.

As this seems fairly low controversy, this insta-stabilizes the field.
In prior versions of Cargo, users will get an "unused manifest key"
warning.
For packags where this is set by `cargo publish`, the warning will be suppressed and things will work as normal.
For `cargo vendor`, the same except there will be some churn in the
vendored source as this field will now be set.
For local development, it should be rare to set `autolib` so the lack of
error by discovering a file when this is set shouldn't be a problem.

Fixes rust-lang#14476
bors added a commit that referenced this issue Sep 27, 2024
feat(toml): Add `autolib`

### What does this PR try to resolve?

PR #5335 added `autobins`, etc for #5330.  Nowhere in there is
discussion of `autolib`.

Cargo script disables support for additional build-targets by disabling
discovery.
Except we don't have a way to disable discovery of `autolib`, leading to #14476.
By adding `autolib`, we can continue in that direction.

This also allows us to bypass inferring of libs on published packages,
like all other build-targets which were handled in #13849.

Fixes #14476

### How should we test and review this PR?

### Additional information

As this seems fairly low controversy, this insta-stabilizes the field.
In prior versions of Cargo, users will get an "unused manifest key"
warning.
For packages where this is set by `cargo publish`, the warning will be suppressed and things will work as normal.
For `cargo vendor`, the same except there will be some churn in the
vendored source as this field will now be set.
For local development, it should be rare to set `autolib` so the lack of
error by discovering a file when this is set shouldn't be a problem.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-hard Experience: Hard
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants