r? @matklad

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

I'm wondering if all the advanced-env config tests need this as well (everything that calls new_config()). It's not clear to me in what situation this is actually needed, though.

probably, update on the way.

Question: why these tests pass Cargo's own CI, which uses stable, in the first place?

@matklad I don't fully understand ether, the best explanation so far is #5648 (comment)

Hm I agree with @matklad that it's a little worrisome how these tests are passing today. Could we perhaps:

• Unconditionally set the release channel to "stable" for all tests
• Have specific tests opt-out to using the nightly/dev version of Cargo

That way we should proactively catch this and it's clear which tests are opting in to nightly?

@alexcrichton I do not, at this time, know this stuff well enough to make that happen. When we have a plan of action, someone could give mentoring instructions, or just submit an alternative PR. I made this PR as a quick fix, I was unclear if this was a crises (like blocking the 1.27 release.) that needed a fix before the knowledgeable people returned.

@Eh2406 oh no worries! I can try to help guide you through this if you want as well.

It actually looks like we already do this for everything using project(...) and invoking the Cargo executable. In that sense this may be exclusive to these tests which aren't using that and may just mean that new_config needs to change?

Yes I believe that is true. I.E. "integration test" that use ProcessBuilder require masquerade_as_nightly_cargo to use nightly only things. But they do that by setting the env for the new spawned process. The problem is for tests that do not spawn a process, how do we default them all to "stable" and how do we override it when needed without rass conditions between tests.

Indeed yeah, I'd imagine that a method could be added to Config to override the channel?

Can we just call set_var in that method or will other test get to see that we changed it?

One idea to ensure everything tests on stable by default is to change this line to "stable":

I tested it, and only the tests fixed in this PR fail. That way, unittests that need nightly will be be explicit (either by overriding the channel or just ignoring the test).

@ehuss dose that means that cargo built from source does not allow nightly things?

@ehuss dose that means that cargo built from source does not allow nightly things?

Ah, yea, that probably would be annoying. Nevermind. 😛

I tried .unwrap_or_else(|| String::from(if cfg!(test) {"stable"} else {"dev"} )) but tests in the cfg!(test) is false for tests in cargo\tests directory. So that is a nogo.

Also this is buggy as nightly_features_allowed != is_nightly. I was wrong. is_nightly checks if rustc can compile nightly things. We want nightly_features_allowed that checks if cargo can use nightly things. But it is not worth fixing If we are going to use a different approach.

Could a global variable be added to src/cargo/core/features.rs with a function like disable_nightly_features()? That way all these tests would call that function (or the helper all these tests use would call that function). I think that'd be relatively uninvasive?

@alexcrichton I don't quite understand your suggestion. Witch is ok, you could elaborate or make an alternative PR, whichever is easier for you.

The design constraints I see are:

1. test see "stable" unless they opt out. (or we are likely to regress on this with the next nightly thing.)
2. test do not get to see the setting from previous test. (or debugging with bee a nightmare.)
3. a cargo build gives a binary that has nightly things. (or developing new feachers with be hard.)

So I think you are suggesting a global mutable variable, that defaults to "dev" (so we get 3) and can be changed to "stable" using the helper function disable_nightly_features. Isn't that the wrong default for 1, and how do we ensure it is reset between test for 2?

Ah yes those constraints are indeed correct! I think my idea wasn't quite what we wanted but I whipped this up -- https://gist.github.com/alexcrichton/9c878e9d1c81a1956af9323657dacf2b -- which is somewhat similar. I think that should solve everything?

Clever!

Updated to match your patch, + some additional comments.

@bors: r+

Thanks so much!

📌 Commit 75bb190 has been approved by alexcrichton

⌛ Testing commit 75bb190 with merge 94f7058...

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