devel/freebsd-gcc13/Makefile
3	This should use `DISTVERSION` if possible.
16	Why is the port not supported? Try to give a more meaningful description.
93–95	This could be solved more elegantly.
111–133	This looks like it could also be solved in a more declarative manner without the big .if cascade.
139	Do not silence commands in install targets (cf. § 5.17 Porter's Handbook).
devel/freebsd-gcc13/files/patch-i686
10	This should probably check if the system was actually built for i486 or be a port option. It is still possible to configure FreeBSD for i486.
devel/freebsd-gcc13/pkg-descr
3	This description sounds a bit outdated. We no longer build base with gcc. Also see if you can make it at least three lines. E.g. mention which languages the port supports.

jhb marked an inline comment as done.Nov 13 2023, 9:24 PM

jhb added inline comments.

devel/freebsd-gcc13/Makefile
16	Commit acd121613431056e253039951f66b062a4104746 for devel/freebsd-gcc12 has more details but not sure there is a better summary than this one line. This type of language is used fairly commonly in IGNORE_FreeBSD_12 already (3 other uses of this phrase, several others are just "not supported < 13.1").
93–95	I'm not sure that is more readable, especially if we added this for more arches (namely aarch64) in the future.
111–133	It's almost 'PLIST+= ${.CURDIR}/pkg-plist.${TARGETARCH}' but not quite. There are several special cases.
devel/freebsd-gcc13/files/patch-i686
10	No. We require 64-bit atomics in userland for i386 starting with FreeBSD 11.x. Same is true in clang. This patch is already present in the devel/freebsd-gcc12 port and one I will likely upstream to GCC at some point.
devel/freebsd-gcc13/pkg-descr
3	The sole purpose of this port is to build the base system via CROSS_TOOLCHAIN. That's why it is different than lang/gcc*. For example, it defaults to using the base system libc++ instead of libstdc++ from GCC.

jhb marked an inline comment as done.

jhb added inline comments.

devel/freebsd-gcc13/Makefile
111–133	I have tried to clean this up for the existing ports in D42590 and have similar changes for this port if that review is ok'd.
139	Humm, that rule is newer than the original version of this port. I appreciate its goal, but in this case this isn't installing new files, but removing things that were previously installed. Still, rather simple to remove the silence.

devel/freebsd-gcc13/Makefile
16	You could write something like “missing support, see acd1216” so an interested developer can find the reason for the lack of support. This type of language is used fairly commonly in IGNORE_FreeBSD_12 already (3 other uses of this phrase, several others are just "not supported < 13.1"). Doesn't make it any better.
93–95	Your call. I prefer the declarative style as it's easier to deal with when the special cases pile up.
111–133	Thanks, I have responded to your proposed changes over at the other review.
139	Especially for removals. Imagine you're wondering while a file that is clearly being built and installed doesn't show up in the package. You won't be able to tell from the build log that it's being removed last minute. This can be very frustrating to debug. Just make the commands visible so there's no surprise.
devel/freebsd-gcc13/pkg-descr
3	That sounds reasonable but is not perfectly clear from pkg-descr. Though I can see how it is meant that way.

devel/freebsd-gcc13/Makefile
16	You could write something like “missing support, see acd1216” so an interested developer can find the reason for the lack of support. Should use 12 chars acd121613431

jhb marked an inline comment as done.Nov 14 2023, 4:12 PM

jhb added inline comments.

devel/freebsd-gcc13/pkg-descr
3	This description is certainly been inherited from several generations of this port (back to devel/powerpc64-gcc). Would this be clearer: GCC, the GNU C/C++ Compiler, customized to build the FreeBSD base system.

devel/freebsd-gcc13/Makefile
16	Perhaps `FreeBSD 12.x cannot be built with this version of GCC`?

devel/freebsd-gcc13/Makefile
16	But then it would still make sense to build gcc on 12.x to build other FreeBSD versions.
devel/freebsd-gcc13/pkg-descr
3	Sounds reasonable. I'd add a note that you should use lang/gcc* for other purposes.

devel/freebsd-gcc13/Makefile
16	Only sort of. GCC hardcodes the target version it is building for at compile time unlike clang which accepts `-target`, so that means if there is a behavior change for a given major version (e.g. requiring 64-bit atomics for x86 for 11+, or when we switched to .init_array by default), you have to build a GCC targeted for that version. Right now these ports use the same target version as the host version, so they only support cross-arch, but technically not cross-version builds. In practice that tends to be good enough most of the time (e.g. there haven't been any target-facing changes in several major versions now, so 13 can build 14, etc.), but it's only fully supported to use the GCC port/package on FreeBSD X to build version X.

Various cleanups

LGTM.

devel/freebsd-gcc13/Makefile
16	Reasonable and possible worth explaining in more detail, too. It is not obvious that only cross-arch is supported.
devel/freebsd-gcc13/files/patch-i686
10	The FreeBSD 13 release notes read like you can still build for i486 and then the toolchain will use the same workarounds as before (i.e. “Users who need to run on 486- or 586-class CPUs need to build their own releases.”) https://www.freebsd.org/releases/13.0R/relnotes/

This revision is now accepted and ready to land.Nov 17 2023, 10:30 PM

devel/freebsd-gcc13/Makefile
16	Probably it is worth expanding pkg-descr further to clarify this.
devel/freebsd-gcc13/files/patch-i686
10	Yes, I believe the intent of that is that you would need to use an explicit `-march` in your CFLAGS (or set CPUTYPE explicitly which does that). Even then you probably have to build with `WITHOUT_LLDB` to get a build that doesn't fail. And that is true for clang as well, not just if you are using GCC as the external compiler. This patch is merely changing the default -march for the compiler and matches what is in clang here: https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Arch/X86.cpp#L111 Actually, it seems for clang the default to i686 is not even conditional on major version, it was just changed for the in-tree clang starting in 11.0 before being merged upstream: https://github.com/llvm/llvm-project/commit/02cfa7530d9e7cfd8ea940dab4173afb7938b831