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

Update mdbook #106520

Merged
merged 1 commit into from
Jan 14, 2023
Merged

Update mdbook #106520

merged 1 commit into from
Jan 14, 2023

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Jan 6, 2023

This updates mdbook from 0.4.21 to 0.4.25. The list of changes is here. The only user-visible changes are some changes around the theme picker, and change to the copy-to-clipboard ignoring hidden lines.

Internally there were some dependency updates and small fixes.

This also updates clap from 4.0.15 to 4.0.32 whose changelog is here. This impacts tools like cargo. I don't see anything particularly noteworthy there, though there are some small user-visible changes.

Unfortunately this required adding a hack for building rustix with a bootstrap tool. The comment explains why. I am unable to think of some other workaround (or even a cleaner way to set the rustflag). Ideas are welcome if you can think of alternatives. I'm struggling to even think of a long-term solution, other than asking projects not to do auto-nightly feature detection.

One medium-term solution is to avoid the clap dependency for the mdbook library (which is how rustix gets pulled in). That is one of my goals for the 0.5 release of mdbook, but that probably won't happen until later this year. It would also require dropping clap from rustbook and using some other means to parse arguments (there's only two options, so it can probably be done manually).

@rustbot
Copy link
Collaborator

rustbot commented Jan 6, 2023

r? @Mark-Simulacrum

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

@rustbot rustbot added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 6, 2023
@rust-log-analyzer

This comment has been minimized.

@ehuss
Copy link
Contributor Author

ehuss commented Jan 6, 2023

Hm, this may be more challenging than I anticipated.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 6, 2023
@Mark-Simulacrum
Copy link
Member

rustix's compilation detection isn't great (https://github.com/bytecodealliance/rustix/blob/main/build.rs#L185). In general I think such attempts are often quite painful for build systems that are managing multiple rustc's or otherwise trying to avoid incorrect usage. For example, if you were trying to do distributed compilation via sccache or similar this would be annoying.

That said, at this point I'm not sure we can prevent their use, it's too wide spread. Nightly feature detection via such hacks is even worse (basically sitting on a ticking bomb that'll go off as soon as nightly feature changes it's API).

I wonder if maybe at minimum having some option in Cargo to run build scripts with a RUSTC_FORCE_STABLE or something might be a "good" workaround. But I can't say I'm enthusiastic about it, definitely needs a lot of plumbing (rustc, cargo, etc).

FWIW, if we don't need features, not setting RUSTC_BOOTSTRAP might be enough for this particular case; I believe we have allow-features and bootstrap as a stopgap for some other usage (cargo unstable features?) which might have other workarounds.

But none of this really makes me happy.

@rust-log-analyzer

This comment has been minimized.

@ehuss ehuss force-pushed the update-mdbook branch 2 times, most recently from d901dc7 to bac021e Compare January 10, 2023 01:03
@ehuss
Copy link
Contributor Author

ehuss commented Jan 10, 2023

@rustbot ready

OK, here's a second attempt. I hit many roadblocks trying to reach this. An overview of how this works:

Instead of passing -Zallow-features via RUSTFLAGS, it is passed via a backdoor to the rustc wrapper. This helps ensure that -Zallow-features is always used by the compiler, even for build scripts like rustix which don't pass any flags to rustc, or for build-dependencies which don't receive RUSTFLAGS.

This is now specified through a new method on the Cargo type. I have also narrowed the scope of which nightly features get enabled (to somewhat make it clearer exactly which tools need which features).

This is moderately risky, since I can't test every dependency of every target, and I can't test every x.py command. However, I have done a lot of testing to arrive at this state, running all the affected tools.

I don't think removing RUSTC_BOOTSTRAP will completely solve the problem here. It is currently needed for -Zbinary-dep-depinfo in all cases. Also, I think it seems like a good goal to keep -Zallow-features for non-stage-0 tools, in order to keep their set of features in check.

We could work with the crate authors to try to come to some other solution, but I don't know if they would be willing to remove the dynamic scanning. Another option is to work on cargo to better inform build scripts which flags they should be looking at, but that's a much harder project.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 10, 2023
@epage
Copy link
Contributor

epage commented Jan 13, 2023

Does removing the color feature sufficient?

@ehuss
Copy link
Contributor Author

ehuss commented Jan 13, 2023

Does removing the color feature sufficient?

It might, but unfortunately there isn't an easy way to turn it off.

@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=never

I remain unhappy, but I don't have better options. This seems like a relatively painless solution in terms of maintaining rustbuild at least.

Long-term I guess we need something like user-opt-in to nightly feature use in a way that doesn't suffer from the problems of Cargo features (i.e., too hard to disable, default-features, etc.).

@bors
Copy link
Contributor

bors commented Jan 14, 2023

📌 Commit 2717f60 has been approved by Mark-Simulacrum

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 Jan 14, 2023
@bors
Copy link
Contributor

bors commented Jan 14, 2023

⌛ Testing commit 2717f60 with merge e9e0908...

@ehuss
Copy link
Contributor Author

ehuss commented Jan 14, 2023

Thanks! Yea, I'm not happy about adding this, either. I'd be up for discussing longer-term solutions. One idea is to somehow combine -Zallow-features and RUSTC_BOOTSTRAP through an environment variable, and have both cargo and rustc honor that natively (and remove the wrapper hack from this PR). Maybe something like:

  • RUSTC_BOOTSTRAP=1 — enable all the things
  • RUSTC_BOOTSTRAP=crate_name,… — enable all the things per-crate
  • RUSTC_BOOTSTRAP=crate_name=feature1;feature2;feature3,… — Enable specific features per crate
  • RUSTC_BOOTSTRAP=*=binary-dep-depinfo,… — enable specific features (globally)
    (And crate-name matching could use glob matching if needed.)

Then, most of the bootstrap tools just set RUSTC_BOOTSTRAP=*=binary-dep-depinfo, rust analyzer could be something like RUSTC_BOOTSTRAP=proc-macro-srv=proc_macro_internals;proc_macro_diagnostic;proc_macro_span. If we'd like to control the nightly features for rustc, then it could be RUSTC_BOOTSTRAP=rustc_*=all to enable all features for rustc crates.

Anyway, just a half-baked idea. Let me know if you'd be interested in pursuing something.

@bors
Copy link
Contributor

bors commented Jan 14, 2023

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing e9e0908 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 14, 2023
@bors bors merged commit e9e0908 into rust-lang:master Jan 14, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e9e0908): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
4.5% [3.4%, 5.5%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.9% [-0.9%, -0.9%] 1
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants