Skip to content

Conversation

@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Apr 14, 2020

Motivation for this change

This is long overdue, @LnL7 Time for you to say "I told you so!" :).

CC @matthewbauer @TravisWhitaker

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built stdenv on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added the 6.topic: vim Advanced text editor label Apr 14, 2020
@Ericson2314
Copy link
Member Author

Seeing that this is such a massive rebuild, on I am all ears on how we can do more nix and less bash to make the inevitable follow-up work easier.

Maybe we could purely concat lists for all those *flags file and write once? The building detection the wrapper script does currently I am not too pleased with.

@Ericson2314 Ericson2314 force-pushed the cxx-wrapper-debt branch 3 times, most recently from 25260c6 to 881d8cc Compare April 14, 2020 02:56
Copy link
Member

Choose a reason for hiding this comment

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

We may need a migration or warning for this.

It may not be actively used though

Copy link
Member

Choose a reason for hiding this comment

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

aliases.nix should be enough.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure what should be aliased?

Copy link
Member

@Mic92 Mic92 Jun 15, 2020

Choose a reason for hiding this comment

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

Not aliased but we need a throw statement in aliases.nix that generates a warning stating that this file has been removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah! sounds good.

Copy link
Member

Choose a reason for hiding this comment

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

This may be used outside by wrappers outside of nixpkgs. might be worth keeping, but just as meta-information.

Copy link
Member Author

Choose a reason for hiding this comment

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

It might be, but I think that's bad :)

Copy link
Member

@matthewbauer matthewbauer Apr 14, 2020

Choose a reason for hiding this comment

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

There's no good way to use libraries that want these c flags without some meta-info like this. For instance bindgen in rust or anything that uses libclang. If we're going to be removing it, we should at least have a replacement.

Copy link
Member

Choose a reason for hiding this comment

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

see #26474 for why this was exported

Copy link
Member Author

Choose a reason for hiding this comment

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

Right I would rather have a more general way to get flags and just this one in particular. The default part (i.e. miy get overriden) is no good too.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should just expose all of these flags in the cc-wrapper ? Something like stdenv.cc.cflags_compile and stdenv.cc.ld_flags

Copy link
Member

@matthewbauer matthewbauer Apr 14, 2020

Choose a reason for hiding this comment

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

that is the pre-substituted values since we don't know the actual values at evaluation

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be nice to uses more Nix so we did know the values. (e.g. have a nix boolean and then assert at run-time it was correct.)

Copy link
Member

Choose a reason for hiding this comment

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

I think that requires Nix-level setup-hooks to work correctly. I think that might be too ambitious for now.

Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking something like /nix-support/get-cflags.sh which does all of this for you.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well getting all the -I flags for other packaged requires that, but not just the std library stuff I think.

Copy link
Member

Choose a reason for hiding this comment

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

Right these should just need the basic c/c++ headers and library paths.

@Ericson2314 Ericson2314 force-pushed the cxx-wrapper-debt branch 2 times, most recently from 9427d9c to de07048 Compare April 14, 2020 15:17
@matthewbauer matthewbauer requested review from 7c6f434c and Mic92 April 14, 2020 19:22
@matthewbauer
Copy link
Member

I think we can also remove some of the old hacks like in https://github.com/NixOS/nixpkgs/pull/26709/files too

@LnL7
Copy link
Member

LnL7 commented Apr 14, 2020

Does that change here? I would expect this to still only include it for clang++. Come to think of it, wouldn't CC=$CXX be a nicer fix for those cases or does that not work?

Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to expose the libc++ of the compiler, similar to libc?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, though null for libstdc++ is rather unfortunate. Oh well, all the more reason to get on splitting up the GCC derivation.

Copy link
Member

@LnL7 LnL7 Apr 15, 2020

Choose a reason for hiding this comment

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

Perhaps for now cc.libcxx = gcc would make sense? Maybe that could also solve cc ? gcc.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm on the fence about this---I do want to get rid of cc.gcc but it's a bit disingenous unless we can somehow filter the other libraries out. Maybe we can leave that for a follow-up PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

#91293 got rid of cc.gcc here.

@matthewbauer
Copy link
Member

Does that change here? I would expect this to still only include it for clang++. Come to think of it, wouldn't CC=$CXX be a nicer fix for those cases or does that not work?

Yeah, you're right - I was forgetting what the issue was there. We still need CXX to be used.

@Ericson2314 Ericson2314 force-pushed the cxx-wrapper-debt branch 2 times, most recently from a7808ca to 995989a Compare April 19, 2020 01:25
@ofborg ofborg bot added the 6.topic: stdenv Standard environment label Apr 19, 2020
@Ericson2314 Ericson2314 changed the title C/C++ compilers: Be sane with standard libraries C++ compilers: Be sane with standard libraries Apr 19, 2020
@Ericson2314
Copy link
Member Author

OK with the last push I have both stdenvs building, so now we can clean up in earnest.

@Mic92
Copy link
Member

Mic92 commented Apr 20, 2020

We also have some tools that consume the c/c++ standard includes: cquery, clang-tools and ccls

@Ericson2314 Ericson2314 changed the base branch from master to staging June 14, 2020 21:44
@Ericson2314 Ericson2314 force-pushed the cxx-wrapper-debt branch 2 times, most recently from eba5f1f to 9e62a88 Compare June 22, 2020 04:21
@Ericson2314 Ericson2314 marked this pull request as ready for review June 22, 2020 04:22
@Ericson2314 Ericson2314 requested a review from jonringer as a code owner June 22, 2020 04:22
@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch. 8.has: clean-up This PR removes packages or removes other cruft 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. and removed 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python Python is a high-level, general-purpose programming language. 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` labels Jun 22, 2020
@Mic92
Copy link
Member

Mic92 commented Jun 22, 2020

We also have some tools that consume the c/c++ standard includes: cquery, clang-tools and ccls

As it turns out those are not affected by this change. I removed cquery last week. Clangd and CCLS only take the libcxx include path as input. I remember to use default_cxx_stdlib_compile for youcompletme at some point, but this was more of a hack and using something like ${llvmPackages.libcxx}/include/c++/v1 would be cleaner.

@Ericson2314
Copy link
Member Author

OK, it evals! I don't think we need any more back-compat stuff ATM, but happy to add things in requested.

@Ericson2314 Ericson2314 merged commit fa54dd3 into NixOS:staging Jun 22, 2020
@Ericson2314 Ericson2314 deleted the cxx-wrapper-debt branch June 22, 2020 14:38
@vcunat vcunat mentioned this pull request Jun 29, 2020
4 tasks
@alyssais alyssais mentioned this pull request Jun 30, 2020
10 tasks
symphorien added a commit to symphorien/nixpkgs that referenced this pull request Feb 3, 2022
NIX_CXXSTDLIB_COMPILE has been removed in NixOS#85189
and NixOS#85189 (comment)
remommends using the files in ${clang}/nix-support to find the correct
flags to pass to libclang.
@rrbutani rrbutani added the 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related label May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related 6.topic: stdenv Standard environment 6.topic: vim Advanced text editor 8.has: clean-up This PR removes packages or removes other cruft 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants