Compiler build depends on libstdc++-static

[kubycsolutions]

Location

https://github.com/rust-lang/rust#building-on-a-unix-like-system

Summary

The README lists a number of packages which have to be present for ./x.py build to succeed.

In my Win10/WSL/fedoraremix environment, I found I had to also install libstdc++-static before compilation could start. It now seems to be running without errors (though with some warnings in the .cpp files on -Warray-bounds, -Wnonnull, and -Wcast-function-type).

This may be a quirk of how this fedora was adapted to run under WSL. But adding that dependency to the instructions seems worth considering; it shouldn't hurt anything and might help some.

[jyn514]

Seems useful :) Happy to take a PR.

[mati865]

It's a bit unfortunate default for local development but you can change it with config.toml by setting this line to false:

rust/config.toml.example

Line 92 in 77e7b74

#static-libstdcpp = true

(Also remove .example from the name).

[jyn514]

Changing the default also seems fine; maybe not the global default, but the named profiles in src/bootstrap/defaults at least.

[kubycsolutions]

OK, folks, I need a decision on that. This is the default build configuration, which beginners will not look inside until later. It would seem odd to me to have the basic README.md immediately tell folks they have the option of changing this one parameter. I think we would want their first build to succeed with as little manual work as possible.

So our choices seem to be to add the static C++ library to the dependencies (and let them decide not to use it later, if so inclined), or to ensure x.py build runs without it -- either by changing config.toml.example, or by changing the default in the code (stylistically better than having the example override the defaults), possibly on a selective basis as @jyn514 proposed.

The first option is strictly a one-line text change, which I can submit a pull request for. Changing the build's defaults would need to either wait for me to dive significantly deeper into this codebase, or be done by someone else.

As a raw newbie in Rust, I don't think I should be the one to make that decision -- it's stylistic as much as coding, and I don't yet have any sense of the community's preferences or have a strong opinion either way.

[jyn514]

I don't know the history of this option. @jonhoo @mati865 why would you ever want to turn it on, given that it requires you to have an extra dependency turned on?

@kubycsolutions I think adding a note to the readme that you need the dependency is a good change in the meantime.

[mati865]

It's useful for dist builds because with that option disabled your binaries will depend on compatible libstdc++. Which might be hard to supply when building rustc for other system.
For local development I think there is no visible differences (there might be 1% perf improvement with static lib but it doesn't really matter).

[jyn514]

Ok, cool. Sounds like we should disable it by default and reenable it in dist builders. @kubycsolutions are you interested in making that change? :)

Mentoring instructions:

• remove

  rust/src/bootstrap/config.rs

  Line 784 in b0f3940

  config.llvm_static_stdcpp = true;

• flip

  rust/config.toml.example

  Line 92 in b0f3940

  #static-libstdcpp = true

• add --set build.static-stdcpp in

  rust/src/ci/run.sh

  Line 65 in 2afca78

  RUST_CONFIGURE_ARGS="$RUST_CONFIGURE_ARGS --set rust.codegen-units-std=1"

[kubycsolutions]

Soitenly. Expect pull request a bit later. I need to check your practice -- fork first, or for something this simple just submit PR? (Most projects prefer fork. I tend to cheat on my own.)

(Woke up, got my eyes uncrossed, edited, noted that gettext needs to be added to the dependencies in the README.md and edited that, build in progress.)

[kubycsolutions]

Might be overkill for this, but for future reference: What's the recommended smoketest before a PR against rust? (I know the compiler built, and I believe the change is correct, but... (Belt, suspenders, helium balloons, drones.)

[jyn514]

@kubycsolutions x test tidy is a good smoke test, or if you're refactoring code, x check. If you've built the compiler successfully that's more than enough; PR CI will catch most of the rest.

[kubycsolutions]

Good 'nuff.

[kubycsolutions]

OK. Fixed the missing quote.

The pull request is still failing its sanity checks. Current failure seems to be
RuntimeError: failed to find config line for static-stdcpp

It isn't entirely clear which file it's complaining about, but I'm guessing that it wanted config.llvm_static_stdcpp = true; flipped rather than removed. Trying that.

[kubycsolutions]

Question: When building under WSL/fedora, I'm seeing the warning

CMake Warning (dev) at /usr/share/cmake/Modules/GNUInstallDirs.cmake:239 (message):
  Unable to determine default CMAKE_INSTALL_LIBDIR directory because no
  target architecture is known.  Please enable at least one language before
  including GNUInstallDirs.
Call Stack (most recent call first):
  /home/keshlam/fork/rust-103606/src/llvm-project/llvm/cmake/modules/LLVMInstallSymlink.cmake:5 (include)
  tools/llvm-ar/cmake_install.cmake:56 (include)
  tools/cmake_install.cmake:49 (include)
  cmake_install.cmake:78 (include)
This warning is for project developers.  Use -Wno-dev to suppress it.

Expected, or did I miss a setup step?

(I'd assume the latter, but the C++ code throws a lot of warnings while compiling so I'm not expecting a 100% warning-free build.)

[kubycsolutions]

Currently getting following from PR checking. Looking into why.

    configure_section(sections[section_key], section_config)
  File "/checkout/src/bootstrap/configure.py", line 457, in configure_section
    raise RuntimeError("failed to find config line for {}".format(key))
RuntimeError: failed to find config line for static-stdcpp

Trying x clean before rebuilding to see if someone missed declaring a dependency.

[kubycsolutions]

Another beginner question: Tidy called me out on formatting (tab character). Thought I'd just run cargo fmt over the whole project on general principles. It's giving me the following two warnings. Expected?

Warning: can't set `version = Two`, unstable features are only available in nightly channel.
Warning: can't set `ignore = IgnoreList { path_set: {"compiler/rustc_codegen_cranelift/scripts", "src/tools/cargo", "src/tools/rust-installer", "src/doc/reference", "src/doc/nomicon", "/build/", "src/doc/embedded-book", "/vendor/", "src/doc/edition-guide", "/*-build/", "library/portable-simd", "library/stdarch", "src/doc/book", "compiler/rustc_codegen_gcc", "src/doc/rustc-dev-guide", "/build-*/", "src/llvm-project", "src/tools/miri", "src/test", "src/tools/rls", "src/doc/rust-by-example", "src/tools/rustfmt", "compiler/rustc_codegen_cranelift/y.rs", "src/tools/clippy", "src/tools/rust-analyzer", "compiler/rustc_codegen_cranelift/example", "library/backtrace"}, rustfmt_toml_path: "" }`, unstable features are only available in nightly channel.

(Tidy also changed a heck of a lot more files than I expected, so I've reverted that. But trying to run rustfmt over just bootstrap/config.rs gave me the same warnings, and I'd sorta like to understand what's going on for future reference.)

((I noticed in passing rustfmt doesn't fully grok the

let x=foo
       .bar()
       .baz()
       .murph();

style; it wants to move .bar() up onto the let line. Not a big deal, just a minor feature to be added someday.))

[kubycsolutions]

Autocheck is still failing, and I'm not sure what's causing the issue:

 configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-r ...
    raise RuntimeError("failed to find config line for {}".format(key))

[jyn514]

@kubycsolutions I suggest posting questions about how to implement your change on https://rust-lang.zulipchat.com/#narrow/stream/326414-t-infra.2Fbootstrap, Github is annoying to use for conversations.

That said, I think your problem here is that the key is named static-libstdcpp, not static-stdcpp like you have in the docker file.

[jonhoo]

Sorry it took me forever to get back to this!

Note that the default was specifically changed to statically build against libstdc++ in #94832. The reasoning was that it was already the default whenever llvm-tools was included, which seems fairly common, and so we didn't want to suddenly change the default (though aiui, the PR here does). The second part of the reasoning was that the upstream Rust build is statically linked, so that's probably what folks expect, and not statically linking stdc++ makes it decently likely that users of the build (if you ever send it to another host) will run into problems if their stdc++ is older than what was on the build host, which is a pain to work around.

[jyn514]

Hmm, we could gate the default on channel = dev, maybe. I think I should have a larger conversation with @Mark-Simulacrum about defaults based on the channel, I think defaulting to profile = user for a non-dev channel would sidestep a lot of these issues.

[RalfJung]

Most people working with x.py and developing rustc won't build their own LLVM, x.py will download the one from CI for them. Did this here also change the way that that LLVM is built? What does this change even mean for that setting? It will still affect what rustc_llvm does, but obviously it won't affect how LLVM is built...

[kubycsolutions]

The intent was that it would leave LLVM unchanged, affecting only default build of the full compiler. I must admit I haven't done before-and-after comparison of the LLVM to validate that.

…

-- /_ Joe Kesselman (he/him/his) -/ _) My Alexa skill for New Music/New Sounds fans: / https://www.amazon.com/dp/B09WJ3H657/ () Plaintext Ribbon Campaign /\ Stamp out HTML mail!

________________________________ From: Ralf Jung ***@***.***> Sent: Sunday, November 13, 2022 1:16:31 PM To: rust-lang/rust ***@***.***> Cc: Joseph Kesselman ***@***.***>; Mention ***@***.***> Subject: Re: [rust-lang/rust] Compiler build depends on libstdc++-static (Issue #103606) Most people working with x.py and developing rustc won't build their own LLVM, x.py will download the one from CI for them. Did this here also change the way that that LLVM is built? — Reply to this email directly, view it on GitHub<#103606 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AOKT7A5MY44Y47LCOWDZVWTWIEV77ANCNFSM6AAAAAARPQE7HU>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[kubycsolutions]

This started as a suggestion to just add static-libstdcpp to the list of dependencies in the README, rather than having the user go thru discovering its absence only by building. We could still discard my change set and go back to that, if it's the better answer I won't be offended.

…

-- /_ Joe Kesselman (he/him/his) -/ _) My Alexa skill for New Music/New Sounds fans: / https://www.amazon.com/dp/B09WJ3H657/ () Plaintext Ribbon Campaign /\ Stamp out HTML mail!

________________________________ From: Jon Gjengset ***@***.***> Sent: Sunday, November 13, 2022 1:02:20 PM To: rust-lang/rust ***@***.***> Cc: Joseph Kesselman ***@***.***>; Mention ***@***.***> Subject: Re: [rust-lang/rust] Compiler build depends on libstdc++-static (Issue #103606) Sorry it took me forever to get back to this! Note that the default was specifically changed to statically build against libstdc++ in #94832<#94832>. The reasoning was that it was already the default whenever llvm-tools was included, which seems fairly common, and so we didn't want to suddenly change the default (though aiui, the PR here does). The second part of the reasoning was that the upstream Rust build is statically linked, so that's probably what folks expect, and not statically linking stdc++ makes it decently likely that users of the build (if you ever send it to another host) will run into problems if their stdc++ is older than what was on the build host, which is a pain to work around. — Reply to this email directly, view it on GitHub<#103606 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AOKT7A5JZ3JPM65XCCXZYV3WIEUKZANCNFSM6AAAAAARPQE7HU>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[RalfJung]

To be clear I am happy about this change for entirely unrelated reasons.^^ It brings me one step closer to having rustc_llvm not be rebuilt when I share a build cache between the flatpak sandbox and the host.

I just don't understand why this helps, since I figured either way I must link to the downloaded LLVM. There is logic in bootstrap that prevents one from (un)setting LLVM options, but it seems when the default changes that still somehow has an effect...

[jyn514]

@RalfJung see #104289 for how we got the downloaded llvm to remain unchanged while still changing the default

[RalfJung]

Looks like a wrong link? I think you mean #104245.

But bootstrap then still invokes rustc_llvm as if llvm_static_stdcpp = false -- but the artifact we link against was built with llvm_static_stdcpp = true. Or am I misunderstanding?

[jyn514]

That might be a bug ... Can you open an issue?

[RalfJung]

What is a bug? I don't know what I am talking about here.^^ I just observe that, if I understood the code correctly, default_opts is used when using LLVM from CI. There are checks that will error if any of these settings is locally changed.

[mati865]

But bootstrap then still invokes rustc_llvm as if llvm_static_stdcpp = false -- but the artifact we link against was built with llvm_static_stdcpp = true. Or am I misunderstanding?

We build dist with static libstdc++.

[RalfJung]

Yes. And then local bootstrap uses the default value for llvm_static_stdcpp which is now false. So there is a mismatch. Right?

It seems to work though...

[jyn514]

@RalfJung the defaults are mostly ignored when using downloaded LLVM. Instead we use the settings LLVM was built with. The logic is non-trivial but unless you've run into issues I'm inclined to think there's no bug here.

[RalfJung]

I can say for sure that changing the default of llvm_static_stdcpp changed some build details when using LLVM from CI - it no longer tells the rustc_llvm build script about where the static stdcpp is located. I am happy about the change, but the default is definitely not getting ignored.