Skip to content

Conversation

onur-ozkan
Copy link
Contributor

@onur-ozkan onur-ozkan commented Dec 19, 2024

It seems that cargo can't conditionally propagate features if they were enabled by default on the target crate, but disabled with default-features = false in the current/parent crate.

Fixes #118129

It seems that cargo can't conditionally propagate features
when `default-features` is set to `false`.

Signed-off-by: onur-ozkan <work@onurozkan.dev>
@rustbot
Copy link
Collaborator

rustbot commented Dec 19, 2024

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 19, 2024
@jieyouxu
Copy link
Member

That is... weird. I'll take a look tmrw.
r? jieyouxu

@RalfJung
Copy link
Member

It seems that cargo can't conditionally propagate features when default-features is set to false.

What do you mean by this?

@onur-ozkan
Copy link
Contributor Author

This does not work.

@RalfJung
Copy link
Member

It does seem to work sometimes though? Only particular ./x.py invocations are broken.

@onur-ozkan
Copy link
Contributor Author

I couldn't see anything that could be related in bootstrap.

@RalfJung
Copy link
Member

Note that if this truly does not work you'll have to also change it here

rustc_index = { path = "../rustc_index", default-features = false }
rustc_macros = { path = "../rustc_macros", optional = true }
rustc_serialize = { path = "../rustc_serialize", optional = true }
rustc_span = { path = "../rustc_span", optional = true }
tracing = "0.1"
# tidy-alphabetical-end
[features]
# tidy-alphabetical-start
default = ["nightly", "randomize"]
# rust-analyzer depends on this crate and we therefore require it to built on a stable toolchain
# without depending on rustc_data_structures, rustc_macros and rustc_serialize
nightly = [
"dep:rustc_data_structures",
"dep:rustc_feature",
"dep:rustc_macros",
"dep:rustc_serialize",
"dep:rustc_span",
"rustc_index/nightly",
]

and in the dozen other places where we do the same thing.

I think the first step would be to reproduce this in a minimal example and file a cargo issue, if this is really the problem.

@onur-ozkan
Copy link
Contributor Author

Yeah, I saw those too. This PR was kind of a hotfix for the problem you experienced to improve the developer experience. I think it's worth filing an issue on the cargo side, will do that today.

@RalfJung
Copy link
Member

Hm yeah this PR does fix that problem... I have no idea why, given that the same pattern is still used in a whole bunch of other places.

@jieyouxu
Copy link
Member

I think it's worth filing an issue on the cargo side, will do that today.

I think we probably should wait until we hear back from T-cargo before merging this (even as a hotfix)... I don't quite understand this current behavior either.

@jieyouxu

This comment was marked as off-topic.

@jieyouxu
Copy link
Member

jieyouxu commented Dec 20, 2024

Ok, I did some more testing. This PR's changes notably still passes tests/run-make/rustc-crates-on-stable, i.e. the compiler crates that need to compile on stable still continues to compile on stable (more or less). AFAICT it also preserves the behavior of the non-nightly-ness of rustc_index_macros as a dep for rustc_index. I tried something different like making rustc_index_macros truly optional, and while that also addresses the original report it also regresses tests/run-make/rustc-crates-on-stable (as expected).

@onur-ozkan
Copy link
Contributor Author

Yeah, I saw those too. This PR was kind of a hotfix for the problem you experienced to improve the developer experience. I think it's worth filing an issue on the cargo side, will do that today.

I tried to create a simple reproduction in a new cargo workspace but I couldn't reproduce it. I am not sure what is the special case in compiler tree, but I don't want to spend more time on this since there are alternative x invocations and we already have a fix (which is this PR) for the broken one.

@jieyouxu
Copy link
Member

Yeah, I also tried to make a smaller repro but failed.

@bors r+ rollup=iffy

@bors
Copy link
Collaborator

bors commented Dec 20, 2024

📌 Commit e151148 has been approved by jieyouxu

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 20, 2024
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Dec 20, 2024
…ieyouxu

update `rustc_index_macros` feature handling

It seems that cargo can't [conditionally propagate features](https://github.com/rust-lang/rust/blob/214587c89d527dd0ccbe1f2150c737d3bdee67b0/compiler/rustc_index/Cargo.toml#L20) if they were enabled by default on the target crate, but disabled with `default-features = false` in the current/parent crate.

Fixes rust-lang#118129
dianqk added a commit to dianqk/rust that referenced this pull request Dec 21, 2024
…ieyouxu

update `rustc_index_macros` feature handling

It seems that cargo can't [conditionally propagate features](https://github.com/rust-lang/rust/blob/214587c89d527dd0ccbe1f2150c737d3bdee67b0/compiler/rustc_index/Cargo.toml#L20) if they were enabled by default on the target crate, but disabled with `default-features = false` in the current/parent crate.

Fixes rust-lang#118129
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 21, 2024
Rollup of 6 pull requests

Successful merges:

 - rust-lang#131072 (Win: Use POSIX rename semantics for `std::fs::rename` if available)
 - rust-lang#134325 (Correctly document CTFE behavior of is_null and methods that call is_null.)
 - rust-lang#134526 (update `rustc_index_macros` feature handling)
 - rust-lang#134581 (Bump Fuchsia toolchain for testing)
 - rust-lang#134607 (on pair → on par)
 - rust-lang#134608 (Move test rust-lang#93775 to crashes)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 22, 2024
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#131072 (Win: Use POSIX rename semantics for `std::fs::rename` if available)
 - rust-lang#134325 (Correctly document CTFE behavior of is_null and methods that call is_null.)
 - rust-lang#134526 (update `rustc_index_macros` feature handling)
 - rust-lang#134581 (Bump Fuchsia toolchain for testing)
 - rust-lang#134607 (on pair → on par)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e461a3f into rust-lang:master Dec 22, 2024
6 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 22, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 22, 2024
Rollup merge of rust-lang#134526 - onur-ozkan:nightly-feat-rustc, r=jieyouxu

update `rustc_index_macros` feature handling

It seems that cargo can't [conditionally propagate features](https://github.com/rust-lang/rust/blob/214587c89d527dd0ccbe1f2150c737d3bdee67b0/compiler/rustc_index/Cargo.toml#L20) if they were enabled by default on the target crate, but disabled with `default-features = false` in the current/parent crate.

Fixes rust-lang#118129
@onur-ozkan onur-ozkan deleted the nightly-feat-rustc branch December 22, 2024 05:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bootstrap sometimes fails to build miri because of "nightly" feature confusion
6 participants