Accept rustflag equivalent to no_version

[dtolnay]

Structopt currently looks at the environment variable $CARGO_PKG_VERSION to determine a version for the command line app. In non-Cargo environments this environment variable doesn't make sense and we would prefer to use no_version by default everywhere.

I would recommend for structopt-derive to accept a rustflag cfg that disables the check for CARGO_PKG_VERSION and assumes no_version by default.

RUSTFLAGS='--cfg structopt_no_version' cargo build ...

If a version = "..." attribute is present, that should take precedence over structopt_no_version.

Implementation-wise this would look like #[cfg(structopt_no_version)] and #[cfg(not(structopt_no_version))] codepaths in structopt-derive.

cc @bolinfest

[CreepySkeleton]

@dtolnay this was discussed in #217 , you may want to take a look. Also, if no_version flag is present structopt doesn't look at CARGO_PKG_VERSION at all - that's the point of this flag. If it does - that's a bug, can you report that?

[dtolnay]

@CreepySkeleton I read that thread and would still like to see the improvement I suggested. The discussion in #217 did not cover what should happen if CARGO_PKG_VERSION is not set.

My codebase has a lot of structopt-based clis in a monorepo that has no concept of versions, and it would be great for the build system to be able to set a rustflag so that we avoid writing dozens of #[structopt(no_version)].

[TeXitoi]

It should be possible to not generate a version call if the env variable is not setted.

[CreepySkeleton]

@dtolnay thanks for clarifying, now I see your point. But this custom-cfg solution looks kind of hacky.

@TeXitoi I'd rather keep things explicit.

I suggest feature gated behavior:

[features]
no_default_version = []

It's not that flexible - you can't just tweak behavior on the fly, just by an env var - but it still covers use cases like @dtolnay described quite well. @dtolnay what do you think?

[dtolnay]

@CreepySkeleton I think cfg(structopt_no_version) is the most appropriate way to implement this. The lack of a concept of versions is a property of the build system, not of an individual cli's dependency on structopt. The same cli may be built at different times with a build system that has versions and a build system that doesn't have versions. The latter kind will be able to define structopt_no_version globally.

[CreepySkeleton]

@dtolnay Out of curiosity: if your cli is able to set env variables why can't you just

CARGO_PKG_VERSION=0.0.0 cargo build...

In any case, I think RUSTFLAGS should be left alone save for experimental hacking or inter-compiler stuff. I've had enough interesting experience with cmake, make and friends to state with full responsibility - internal-ish variables should not be touched until absolutely necessary.

I propose STRUCTOPT_NO_DEFAULT_VERSION environment variable - if it's set to 1 or true structopt does not generate versions unless explicitly requested.

[dtolnay]

if your cli is able to set env variables why can't you just

@CreepySkeleton, not having versions is different than having a version number of 0.0.0. They do different things when running --help, for example.

internal-ish variables should not be touched until absolutely necessary.

No internal-ish variables are involved. The rustc --cfg flag is a totally supported and public flag. It is the top thing in rustc --help after --help itself:

$ rustc --help
Usage: rustc [OPTIONS] INPUT

Options:
    -h, --help          Display this message
        --cfg SPEC      Configure the compilation environment
...

Rustc exposes these flags to source code as #[cfg(name_of_flag)] which is a stable and supported language feature.

[CreepySkeleton]

@dtolnay

They do different things when running --help, for example.

Right, didn't think about that.

It is the top thing in rustc --help after --help itself:

That awkward moment when you realize that you are the one who haven't read docs...

Then I have no more objection and absolutely agree.
@TeXitoi Your opinion?

[TeXitoi]

Doesn't the cfg flag also work as feature flag?

[CreepySkeleton]

Doesn't the cfg flag also work as feature flag?

Just the opposite - feature may act as --cfg via --cfg(feature = "no_default_version"), --cfg cannot work as a feature. @dtolnay Am I right here?

Actually, my objection against --cfg is mostly about its inconsistency with other configurations. If some option is the property of the crate in question people use feature. If the option is property of build environment people tend to use environment variables (I'd vote in favor of this path). Custom --cfgs are mostly defined in build.rs file, they tend to stay private. I mean they aren't exposed and aren't supposed to be used by users.

@dtolnay Could you please explain why you prefer a cfg flag over an env variable?

[dtolnay]

@CreepySkeleton, reliance on environment variables is very bad for build cache systems. The core problem is deciding when to rebuild a crate vs use a cached build. Do you rebuild if anything in the env is changed? In practice that makes a cache mostly useless because environments vary a lot across machines for insignificant reasons. Do you use cache despite env changes? That means you'd be getting builds that silently nondeterministically disrespect important environment variables. Since there is no way to introspect what environment variables a macro looks at, it's impossible to have a useful deterministic cache. Cargo has big problems with this too; it prioritizes use of the local cache over correctness, so the environment is not taken into account when deciding whether to rebuild.

In contrast, --cfg is very easy to cache correctly. Is the rustc invocation the same? If so, use the artifact from cache.

I suspect that the uses of environment variables you've seen are all examples of people "doing it wrong".

[CreepySkeleton]

@dtolnay Sorry for the late reply, I needed time to think it over.

Well, it seems I have no more objection. We definitely can implement this. I also very much appreciate your in-detail explanation and reasoning, special thanks for cargo inside. If only all feature requests were like that...

@TeXitoi What is your opinion on this matter?

[TeXitoi]

If it can be used with cfg flags and feature, I'm OK with such an enhancement.

[TeXitoi]

But I still don't understand why you don't want "if the CARGO_PKG_VERSION env var is not setted, don't generate a .version(env!("CARGO_PKG_VERSION")) call". I suppose it will fix this issue.

[dtolnay]

@TeXitoi, I would be happy with that as well.

[CreepySkeleton]

I would be happy with that as well.

@dtolnay I don't understand. You just said

reliance on environment variables is vary bad for build cache systems ... Cargo has big problems with this too; it prioritizes use of the local cache over correctness, so the environment is not taken into account when deciding whether to rebuild

Let's say you first build in "do have version" environment, then environment changes to "have no version" one. CARGO_PKG_VERSION is not set anymore. Cargo doesn't detect this change and refuse to rebuild anything - and you've got some version inherited from "have a version" build artifacts despite environment is different. And vise versa. Am I right here?

@TeXitoi I don't see how --cfg structopt_no_version can be used in place of a feature. If we want both we could just introduce a feature and @dtolnay will be able to use it via --features no_version.

[TeXitoi]

Structopt allready dépends on this env var son that's not "more" bad.

Then I prefer a feature that's the cargo users can use than a cfg.

[CreepySkeleton]

But I still don't understand why you don't want

@TeXitoi

I don't like it. I think if people expect to have a version for their app (I feel like most people do and @dtolnay's case is special here) and they would be upset if structopt will get to silently ignoring things like that.

On the other hand, If your GARGO_PKG_VERSION is not set you're probably know what you're doing and can debug such setbacks.

Don't get it wrong - I will help to implement this if you both come to agreement this is the best way, I just personally don't think it's the right way to go.

[dtolnay]

Let's say you first build in "do have version" environment, then environment changes to "have no version" one. CARGO_PKG_VERSION is not set anymore. Cargo doesn't detect this change and refuse to rebuild anything - and you've got some version inherited from "have a version" build artifacts despite environment is different. And vise versa. Am I right here?

In my case these systems would use totally separate caches so it's not a problem.

Cargo does correctly rebuild when changing environment variables that it knows about, like CARGO_PKG_VERSION. Only non-Cargo variables like STRUCTOPT_SOMETHING would cause broken caching.

I believe @TeXitoi and I are in agreement that #243 (comment) is an acceptable fix.

[CreepySkeleton]

I suggest CARGO_PKG_VERSION=no/no_version. There's a difference between "accidentally not set" and "explicitly set to off".

[dtolnay]

@CreepySkeleton -- downsides of that approach are:

• We aren't using Cargo so having to define Cargo-specific environment variables would be unfortunate.
• That makes the global opt-out impossible to use with Cargo because Cargo will just redefine that variable.

For an "explicitly set to off" build-system-wide setting, cfg(structopt_no_version) would be the way to go.

[CreepySkeleton]

That makes the global opt-out impossible to use with Cargo because Cargo will just redefine that variable.

Makes sense, CARGO_PKG_VERSION is out of question.

Maybe "CARGO_PKG_VERSION is not set" isn't that bad after all...

[CreepySkeleton]

Err, "CARGO_PKG_VERSION is not set" is worse than I thought because of

Cargo will just redefine that variable.

I suggest feature then

[dtolnay]

@CreepySkeleton a feature is not appropriate because whether or not versions exist is a global property of the build system, not a decision that an individual package depending on structopt would make. See #243 (comment). cfg(...) is the mechanism intended for this.

"CARGO_PKG_VERSION is not set" is fine because the motivation for a global no_version setting is for monorepos -- where dozens to hundreds of projects live in the same repo, versions don't exist, and the build system is not Cargo. I believe @TeXitoi is on board with this based on #243 (comment).

[TeXitoi]

1. I think silently skipping the version call when the CARGO_PKG_VERSION is not setted is OK as it's impossible to unset it within cargo, and should not be setted without cargo, and thus there is no ambiguity
2. I'm OK for some cfg flags, but it should also be available as a feature for the cargo users.

I can implement 1., that's 2 lines of code here

structopt/structopt-derive/src/attrs.rs

Line 579 in 14937e8

let version = env::var("CARGO_PKG_VERSION").unwrap_or_else(|_|{

I'm OK for 2, but I'm really not found of features and cfg as it is a mess to properly test and maintain when the number of conditions grows.

[CreepySkeleton]

@TeXitoi

1. It's just about that cargo users wouldn't be able to use this switch at all.

So what do we do?

[TeXitoi]

1 xor 2 xor (1 and 2) I don't care ;-)

[CreepySkeleton]

Aggh!