Skip to content

Remove unsupported options in configure.py #98369

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 1 commit into from
Jul 11, 2022
Merged

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Jun 22, 2022

I've seen people using optimize = false and full-bootstrap = true in the past, without knowing
that they're not recommended. Remove optimize and a few other options that are always a bad idea,
and document that full-bootstrap is only for testing reproducible builds.

@jyn514 jyn514 added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Jun 22, 2022
@rust-highfive
Copy link
Contributor

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 22, 2022
o("extended", "build.extended", "build an extended rust tool set")

v("tools", None, "List of extended tools will be installed")
v("codegen-backends", None, "List of codegen backends to build")
v("build", "build.build", "GNUs ./configure syntax LLVM build triple")
v("build", "build.build", "GNUs ./configure syntax LLVM build triple (unsupported)")
Copy link
Member Author

Choose a reason for hiding this comment

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

this should theoretically be supported, but it doesn't work in practice - I had a conversation a long time ago with someone trying to fix it, but can't find it. Not sure how to replicate locally, I don't have easy access to a musl machine.

Copy link
Member Author

@jyn514 jyn514 Jun 22, 2022

Choose a reason for hiding this comment

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

oh I forgot I still have access to @oli-obk's machine - I'm not sure this is actually the right error though? if anyone knows how to test this happy to try it out

/home/joshua/rustc/build/x86_64-unknown-linux-musl/stage0/bin/cargo: error while loading shared libraries: /nix/store/9rabxvqbv0vgjmydiv59wkz768b5fmbc-glibc-2.30/lib/libc.so: invalid ELF header

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Jun 23, 2022

I am tempted to remove support for them -- they could still be set via --set, right? It seems a little odd to have built-in support for options we don't really want to support in configure.py.

(It's probably not a good idea to remove them from config.toml{,.example}, but I'm not sure showing them in help text makes sense.)

@jyn514
Copy link
Member Author

jyn514 commented Jun 23, 2022

ahh right I forgot --set is a thing.

To some extent I wonder how useful it is to have separate help for configure? We have to keep it in sync with the config.toml options, which in practice means the help gets outdated quite quickly. I wonder if we should just tell people to read config.toml.example instead.

@Mark-Simulacrum
Copy link
Member

I think the common interface for most folks not familiar with Rust (e.g., just building from source for distro/local usage) is through configure; it's never necessary but so long as we have it I think having help for the commonly used options makes sense.

The help messages for configure options are pretty basic I think; if we wanted to add a line of "these all refer to config.toml.example options; please see that for details" I'd be fine with that.

@Mark-Simulacrum Mark-Simulacrum 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 Jun 26, 2022
@jyn514
Copy link
Member Author

jyn514 commented Jul 1, 2022

I am tempted to remove support for them -- they could still be set via --set, right? It seems a little odd to have built-in support for options we don't really want to support in configure.py.

Hmm, I worry this will needlessly break people. I know at least one person (I think in debian?) running full-bootstrap on each release to make sure we don't regress reproducible builds.

I agree we should remove optimize and parallel-compiler though.

@jyn514 jyn514 changed the title Document which options are unsupported in configure.py Remove unsupported options in configure.py Jul 1, 2022
@jyn514 jyn514 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 Jul 1, 2022
@jyn514
Copy link
Member Author

jyn514 commented Jul 1, 2022

Hmm. I see a lot of errors because --build is no longer supported in the configure script. I wonder if we should keep this as a way to enforce that the triple x.py detects is the same you intend to use? or maybe it's not worth keeping these around and we should just remove the --build flags?

@rust-log-analyzer

This comment has been minimized.

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Jul 2, 2022

Hm, actually, I'm not sure we should drop --build. It's sometimes useful for CI and other environments where you could build with multiple build triples, and there's no real way for us to know which one to prefer -- for example, MSVC or mingw on Windows, or 32-bit or 64-bit Linux. I don't think there's much value in us forcing that to be set through --set.

Edit: I see the comments above about it being unsupported, but I think that isn't true? We use it on the i686-gnu CI builder (--build=i686-unknown-linux-gnu is passed to configure) and presumably that builder is running just fine...

@Mark-Simulacrum Mark-Simulacrum 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 Jul 2, 2022
@jyn514
Copy link
Member Author

jyn514 commented Jul 3, 2022

Edit: I see the comments above about it being unsupported, but I think that isn't true? We use it on the i686-gnu CI builder (--build=i686-unknown-linux-gnu is passed to configure) and presumably that builder is running just fine...

I think "unsupported" is not quite the right word - we don't support setting it to a setting other than the one x.py would autodetect. I haven't been able to replicate this unfortunately :( but I definitely remember running into weird build issues with this a while ago ...

anyway, I'll add the flag back since I can't come up with anything more definite than my memory.

@jyn514 jyn514 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 Jul 3, 2022
@Mark-Simulacrum
Copy link
Member

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Jul 7, 2022

📌 Commit 071f7b8 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 Jul 7, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 7, 2022
Remove unsupported options in configure.py

I've seen people using `optimize = false` and `full-bootstrap = true` in the past, without knowing
that they're not recommended. Remove `optimize` and a few other options that are always a bad idea,
and document that full-bootstrap is only for testing reproducible builds.
@matthiaskrgr
Copy link
Member

@bors r- failed in a rollup: #99023 (comment)

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 7, 2022
I've seen people using `optimize = false` and `full-bootstrap = true` in the past, without knowing
that they're not recommended. Remove `optimize` and a few other options that are always a bad idea,
and document that full-bootstrap is only for testing reproducible builds.
@jyn514
Copy link
Member Author

jyn514 commented Jul 10, 2022

@bors r=Mark-Simulacrum rollup=maybe

@bors
Copy link
Collaborator

bors commented Jul 10, 2022

📌 Commit 4151a5b 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 10, 2022
@bors
Copy link
Collaborator

bors commented Jul 11, 2022

⌛ Testing commit 4151a5b with merge 2682b88...

@bors
Copy link
Collaborator

bors commented Jul 11, 2022

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 11, 2022
@bors bors merged commit 2682b88 into rust-lang:master Jul 11, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 11, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (2682b88): comparison url.

Instruction count

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

Max RSS (memory usage)

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 😿 relevant regression found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
3.3% 3.3% 1
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) N/A N/A 0

Cycles

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
3.9% 4.9% 2
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) N/A N/A 0

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change 2

  2. number of relevant changes 2

@jyn514 jyn514 deleted the configure.py branch February 25, 2023 18:32
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.

8 participants