Skip to content

Add configuration stability information #2228

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

Merged
merged 4 commits into from
Dec 17, 2017
Merged

Add configuration stability information #2228

merged 4 commits into from
Dec 17, 2017

Conversation

CAD97
Copy link
Contributor

@CAD97 CAD97 commented Dec 3, 2017

Closes #2227

Stability extracted manually from config.rs. If I read #1998 correctly, the third argument to each line in the create_config! is a boolean indicating un/stable, which is what is now represented here as well.

The unstable_features option is not separately documented, only at the top as how to enable unstable configuration options.

@@ -391,6 +401,7 @@ Indentation of chain

- **Default value**: `"Block"`
- **Possible values**: `"Block"`, `"Visual"`
- **Stable**: ?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This configuration option (chain_indent) wasn't in the create_config! macro (or I missed it).

Copy link
Member

Choose a reason for hiding this comment

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

It has been removed

Copy link
Member

Choose a reason for hiding this comment

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

Could you remove this section from the doc please?

@CAD97
Copy link
Contributor Author

CAD97 commented Dec 3, 2017

Travis failure (macOS) doesn't look like my fault

$ pip install 'travis-cargo<0.2' --user &&
  export PATH=$HOME/.local/bin:/usr/local/bin:$PATH

/Users/travis/.travis/job_stages: line 57: pip: command not found


The command "pip install 'travis-cargo<0.2' --user &&
  export PATH=$HOME/.local/bin:/usr/local/bin:$PATH
  " failed and exited with 127 during .

Copy link
Member

@nrc nrc left a comment

Choose a reason for hiding this comment

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

Thanks for this, it's really good to have these docs. Do we have docs for unstable features in README.md? If not, could you add something there too please?

Stable options can be used directly, while unstable options are opt-in.

To enable unstable options, set `unstable_features = true` in `rustfmt.toml` or pass `--unstable-options` to rustfmt,
and ensure that the environment variable `CFG_RELEASE_CHANNEL` is set to `nightly`.
Copy link
Member

Choose a reason for hiding this comment

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

This is not quite right - users need to be using a nightly toolchain, not setting that env var (which is a compile-time thing, iirc).

Choose a reason for hiding this comment

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

I'm using nightly, but unless I set the env variable I still see these messages:

Warning: can't set `use_try_shorthand = true`, unstable features are only available in nightly channel.
Warning: can't set `error_on_line_overflow = false`, unstable features are only available in nightly channel.
Warning: can't set `error_on_line_overflow_comments = false`, unstable features are only available in nightly channel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I figured it out.

#2235 is a fix to inspect CFG_RELEASE_CHANNEL at rustfmt compile time, whereas in 0.2.17 which was published before that merge, it was checked at runtime. So the rustfmt distributed with the nightly toolchain will be built with support for unstable features, but the one distributed with the beta or stable toolchains won't.

What's the status of CFG_RELEASE_CHANNEL when building crates not as part of the toolchain? (e.g. cargo +nightly install rustfmt-nightly) The main advertised way of acquiring cargo fmt in the README is still via cargo, so we should make sure that it's possible to access the unstable configuration options from there.

@@ -391,6 +401,7 @@ Indentation of chain

- **Default value**: `"Block"`
- **Possible values**: `"Block"`, `"Visual"`
- **Stable**: ?
Copy link
Member

Choose a reason for hiding this comment

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

Could you remove this section from the doc please?

@CAD97
Copy link
Contributor Author

CAD97 commented Dec 4, 2017

I just confirmed that just doing cargo install --path . to install from the current master does not set CFG_RELEASE_CHANNEL to nightly when building (on Windows).

The behavior of 0.2.17 (current version on crates.io) is to check CFG_RELEASE_CHANNEL at runtime. The behavior of master is to check CFG_RELEASE_CHANNEL at compiletime.

As far as I can tell, installing via cargo does not set CFG_RELEASE_CHANNEL, so if a user installs via cargo they currently cannot access unstable configuration options (on master, available on 0.2.17 via setting the env var at runtime).

As far as I can tell, testing for whether a crate is built with a certain rustc version is bound to run into endless edge cases. rustc_version can be used in a build.rs script to check rustc -vV, but just doing cargo +nightly build while the rustup default is stable means that you'll see stable in the build script but be building on nightly.

Doesn't rustfmt only work on nightlies, though? If my intuition is right and this is prepping for the eventual publishing of rustfmt through rustup, may I suggest flipping the condition to !matches!(env!(CFG_RELEASE_CHANNEL), "stable" | "beta")? This should allow checking CFG_RELEASE_CHANNEL when being built officially while still allowing cargo +nightly install rustfmt-nightly to work. Of course, if we can guarantee that rustfmt is always available through rustup then there would be no need for installing from crates.io, but I don't know how plausible that is.


For the time being I've left the documentation in Configurations.md to be accurate to the version currently on crates.io. @nrc, if we get 0.2.18 published where CFG_RELEASE_CHANNEL was checked at compiletime (to be nightly) instead of it checking at runtime like 0.2.17 does, I'll be happy to update the wordage to reflect usage. But for now I think it's more accurate to explain how to get the published version to work.

@topecongiro
Copy link
Contributor

TBH I am not certain about how CFG_RELEASE_CHANNEL works.

Currently we do this:

option_env!("CFG_RELEASE_CHANNEL")
    .map(|c| c == "nightly")
    .unwrap_or(false)

@CAD97 Do you mean that we should not care about the value of CFG_RELEASE_CHANNEL when it's not set? If that's the case we can just flip .unwrap_or(false) to .unwrap_or(true):

option_env!("CFG_RELEASE_CHANNEL")
    .map(|c| c == "nightly")
    .unwrap_or(true)

@nrc Do you think this works?

@CAD97
Copy link
Contributor Author

CAD97 commented Dec 4, 2017

Yes, that is accurate to that wall of text I wrote tired. It makes sense to default to allowing unstable options, then disabling them for stable builds. Any build outside the compiler pipeline will be with the nightly toolchain.

@nrc
Copy link
Member

nrc commented Dec 6, 2017

@topecongiro @CAD97 yeah, I think it makes sense to assume nightly if CFG_RELEASE_CHANNEL is not set

@CAD97
Copy link
Contributor Author

CAD97 commented Dec 6, 2017

Filed #2249 which should enable unstable features enabling when built by cargo but not when built with CFG_RELEASE_CHANNEL != "nightly".

@CAD97
Copy link
Contributor Author

CAD97 commented Dec 17, 2017

Rebased on top of master for CI fix and cleanliness. Also rewords dcd6ed7 slightly to better reflect its new position in history.

If any lints have changed stability since I first issued this PR, ping me and I'll go through and check that the docs are accurate for all of the lints again. Still, it'd be good to see this doc merged.

@nrc nrc merged commit 331124a into rust-lang:master Dec 17, 2017
@nrc
Copy link
Member

nrc commented Dec 17, 2017

Looks good, thank you!

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.

4 participants