Page MenuHomeFreeBSD

Retire WITHOUT_CXX option
ClosedPublic

Authored by emaste on Nov 24 2021, 9:30 PM.
Tags
None
Referenced Files
F86590186: D33108.id.diff
Sat, Jun 22, 4:56 PM
Unknown Object (File)
Sun, Jun 9, 8:20 AM
Unknown Object (File)
Sun, Jun 9, 12:15 AM
Unknown Object (File)
Sun, Jun 9, 12:15 AM
Unknown Object (File)
Sun, Jun 9, 12:15 AM
Unknown Object (File)
Sun, Jun 9, 12:15 AM
Unknown Object (File)
Fri, Jun 7, 7:55 PM
Unknown Object (File)
Thu, Jun 6, 12:49 PM

Details

Summary

Several important base system components are written in C++, and the
WITHOUT_CXX option is not regularly tested and breaks often. Accept
this, and remove the option to build without C++ support.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Agree with the idea in general.

Makefile.inc1
2986

Should this move up into the initial list a few lines earlier instead?

3154

Similarly, just merge this into the list above?

lib/Makefile
187

Should references to these libraries (e.g. ${_libcxxrt}) now just be expanded inline instead?

lib/libproc/Makefile
21

Maybe merge this with the LIBADD below?

tools/build/mk/OptionalObsoleteFiles.inc
3722

I'm a bit surprised there isn't a MK_TOOLCHAIN that covers removing these still? Maybe that would just remove clang though anyway, and not the libraries and headers.

Update with @jhb feedback (except OptionalObsoleteFiles)

emaste marked 4 inline comments as done and an inline comment as not done.Nov 25 2021, 1:57 AM
emaste added inline comments.
Makefile.inc1
3154

done with rewrapping

tools/build/mk/OptionalObsoleteFiles.inc
3722

Hmm, need to double-check.

emaste added a subscriber: dim.
tools/build/mk/OptionalObsoleteFiles.inc
3722

I think there's no knob other than MK_CXX. These will now be installed always and will be just like libc or other /usr/include headers.

4273–4274

but maybe we need to add these somewhere for turning off lib32

tools/build/mk/OptionalObsoleteFiles.inc
4273–4274

No - ${MK_LIB32} == no is handled using find below

Posted a link to this review on the freebsd-hackers@ mailing list.

Discussed libc++ on IRC with @jrtc27. As a followup we may want to move libc++.so.1 from /usr/lib to /lib, and then stop linking devd statically. (See R10:9a4e73fe5ea6f6fe518de8cb3d23cf6e397c8896)

I think moving libc++ to /lib is fine, but should probably be a separate change. Fixing the lack of working MK_TOOLCHAIN in OptionalObsoleteFiles.inc also seems like it would be a separate change?

Hmm, on the OptionalObsoleteFiles.inc front, it does seem that MK_TOOLCHAIN=no does handle some of this as things like MK_CLANG have handling in OptionalObsoleteFiles.inc. It's true that we don't support MK_INCLUDES in OptionalObsoleteFiles.inc, but it might be nice to not regress 'make WITHOUT_TOOLCHAIN=yes delete-old' at least in terms of binaries like /usr/bin/CC and /usr/bin/c++. It may just mean moving those into the MK_CLANG block? Those two are installed today by usr.bin/clang/clang/Makefile. The cc1plus and g++ binaries are specific to GCC and should just be listed in the unconditional ObsoleteFiles.inc if they aren't already. (I just checked and the GCC ones are already handled correctly in ObsoleteFiles.inc).

This revision is now accepted and ready to land.Nov 30 2021, 6:12 PM

I think moving libc++ to /lib is fine, but should probably be a separate change.

That one's D33123.

Fixing the lack of working MK_TOOLCHAIN in OptionalObsoleteFiles.inc also seems like it would be a separate change?

Yes, could be. I cleaned up cc1plus and g++ just now, in 1b9344add475.

We have the following groups of files to deal with:

  1. usr/bin/{CC,c++}
  2. lib/libcxxrt.so.1
  3. usr/{lib,lib32}/libc++*
  4. usr/{lib,lib32}/libcxxrt.*
  5. usr/include/c++/v1/*

MK_TOOLCHAIN indeed seems correct for #1. We can move #5 to a new MK_INCLUDES (with a comment that it's only the C++ includes at present). This leaves only the runtime lib components libcxxrt and libc++, in various combinations.

Yes, could be. I cleaned up cc1plus and g++ just now, in 1b9344add475.

We have the following groups of files to deal with:

  1. usr/bin/{CC,c++}
  2. lib/libcxxrt.so.1
  3. usr/{lib,lib32}/libc++*
  4. usr/{lib,lib32}/libcxxrt.*
  5. usr/include/c++/v1/*

MK_TOOLCHAIN indeed seems correct for #1. We can move #5 to a new MK_INCLUDES (with a comment that it's only the C++ includes at present). This leaves only the runtime lib components libcxxrt and libc++, in various combinations.

I would just move #1 to MK_CLANG since that if you build WITHOUT_CLANG they don't get installed (so they really should be part of MK_CLANG). #2-#4 I'm fine with losing. Maybe someday if we add MK_INCLUDES support that would cover #5. I don't think we need to fix #5 for now though. I would just move #1 into MK_CLANG in this commit and then call it a day.

This revision now requires review to proceed.Dec 1 2021, 9:47 PM
tools/build/mk/OptionalObsoleteFiles.inc
1609–1610

This one is already included in ${MK_TOOLCHAIN}

tools/build/mk/OptionalObsoleteFiles.inc
1609–1610

Now removed in c3f345ae3c0f

This revision is now accepted and ready to land.Dec 2 2021, 5:17 PM
This revision now requires review to proceed.Nov 16 2022, 3:28 PM
brooks added a subscriber: brooks.

It was mentioned on IRC that one reason to keep this is to reduce FS size. IMO if you're at the point wehre this makes a meaningful difference it's time to switch from a subtractive image creation process to an additive one.

It is very occasionally useful to be able to disable CXX things for bootstrapping a new platform with an immature compiler, but there aren't so many things that it's hard to just hack them out of the tree on a temporary basis.

lib/Makefile
217–218
This revision is now accepted and ready to land.Jan 26 2023, 8:58 PM

It was mentioned on IRC that one reason to keep this is to reduce FS size. IMO if you're at the point wehre this makes a meaningful difference it's time to switch from a subtractive image creation process to an additive one.

+1

UPDATING and src.conf.5 updates too?

UPDATING and src.conf.5 updates too?

Staged locally now

This revision was automatically updated to reflect the committed changes.