Mk/bsd.gcc.mk: add support for conditional dependencies for USE_GCC

[gerald]

Summary

This patch is created for https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=211154 and adds support for specifying arguments to USE_GCC.

Test Plan

run with a test port using make test-gcc.

[mat]

I assume USE_GCC= yes:build works, maybe USE_GCC= build could be used instead.

[fabian.freyer_physik.tu-berlin.de]

In D7223#150739, @mat wrote:

I assume USE_GCC= yes:build works, maybe USE_GCC= build could be used instead.

Do you mean as an alias while keeping the same behaviour as is in this patch?

[fabian.freyer_physik.tu-berlin.de]

added aliases for build, run and test as requested by @mat

[gerald]

Thanks for doing this! I have been on medical leave for most of this
months and am still not fully up and running, so would like to defer the
final review to a different (ports) committer or portmgr, but (a) this looks
fine in principle and (b) some specific comments below.

When this goes in, feel free to add Approved by: gerald in addition to that
of the other reviewer(s).

1. Is there any practical need (example?) for the use of TEST_DEPENDS?

I cannot imagine any, but may be missing something, But, if there is none,
let's omit this?

2. Is there any practical need (example?) for the use of test and run?

Unless there is, I suggest to omit "test" and/or "run".

3. In the comments, please refer to "GCC" (uppercase), not "gcc".

[fabian.freyer_physik.tu-berlin.de]

In D7223#152105, @gerald wrote:

Thanks for doing this! I have been on medical leave for most of this
months and am still not fully up and running, so would like to defer the
final review to a different (ports) committer or portmgr, but (a) this looks
fine in principle and (b) some specific comments below.

When this goes in, feel free to add Approved by: gerald in addition to that
of the other reviewer(s).

1. Is there any practical need (example?) for the use of TEST_DEPENDS?

I cannot imagine any, but may be missing something, But, if there is none,
let's omit this?

While I can't think of any current port requiring this, I can imagine it to be useful for any kind of testing of toolkits/toolchains that are used at compile-time and where testing involves compiling things against them. While I admit it's a bit of an edge case, I have seen some (admittedly obscure) library headers not compile correctly using clang++ or g++, respectively. Testing this might be useful. However, I'd agree to not adding GCC to TEST_DEPENDS by default.

2. Is there any practical need (example?) for the use of test and run?

Unless there is, I suggest to omit "test" and/or "run".

Is there any harm in including it? I'd personally tend to including it since in cases when such a need may arise, it does not promote the practice of manually adding to RUN_DEPENDS and/or TEST_DEPENDS.
Example: https://www.freshports.org/devel/stack/

3. In the comments, please refer to "GCC" (uppercase), not "gcc".

No problem here.

[fabian.freyer_physik.tu-berlin.de]

• used GCC instead of gcc in comments
• only add to RUN_DEPENDS and BUILD_DEPENDS by default.

[koobs]

Im +1 on including all *_DEPENDS (including TEST) if nothing else but to keep it consistent (with Python and other Uses) and standardised going forward, as TEST_DEPENDS support in the framework is relatively new. If foo can be a FOO_DEPENDS, I can't see any reason it can't be a TEST_DEPENDS (some piece of software written in * that has its test suite framework written in C)

[mat]

It's missing some error checking, if won't say anything if I do USE_GCC= 4.9:foobar

Mk/bsd.gcc.mk

246–248 ↗

(On Diff #18747)

This looks like debugging info, it should be removed.

254–262 ↗

(On Diff #18747)

This too, debugging. One can see it by running make -V BUILD_DEPENDS, for instance.

[fabian.freyer_physik.tu-berlin.de]

In D7223#152214, @mat wrote:

It's missing some error checking, if won't say anything if I do USE_GCC= 4.9:foobar

Agreed on the error checking, the debug info I'd leave as-is though since it's part of the test-gcc target.

Mk/bsd.gcc.mk

246–248 ↗

(On Diff #18747)

I guess I should have added some more context. This is indeed debug info, but it's part of the test-gcc target, which has the explicit purpose of printing debug information.

254–262 ↗

(On Diff #18747)

see above.

[mat]

Mk/bsd.gcc.mk

246–248 ↗

(On Diff #18747)

Ah, well, you're not using arcanist to upload the diff, so unless you use svn diff --diff-cmd=diff -x -U9999 the review will not get more context and we can't really know what this is all about :-)

[fabian.freyer_physik.tu-berlin.de]

• Added error handling
• Rebased onto r436066
• Added full context

[mat]

I rather agree with @gerald, we try to not add more code that what is in use, if there are no known user of USE_GCC=test and USE_GCC=run, then please, do not add the code for it.

[fabian.freyer_physik.tu-berlin.de]

Remove TEST_DEPENDS stuff.

[gerald]

Thanks for the update, Fabian!

Heads up, my top priority is to get lang/gcc / USE_GCC=yes updated
to GCC 5 in the coming days (which finally appears good to go, then
a small infrastructure improvement that has queued up) and then hope
to get to this finally, hopefully this coming weekend.

Gerald

[fabian.freyer_physik.tu-berlin.de]

In D7223#207883, @gerald wrote:

Heads up, my top priority is to get lang/gcc / USE_GCC=yes updated
to GCC 5 in the coming days (which finally appears good to go, then
a small infrastructure improvement that has queued up) and then hope
to get to this finally, hopefully this coming weekend.

Should rebase this onto those changes at some point later this week?

[gerald]

Thanks for your patience (and sorry for my slowness, still, Fabian)!

This looks good to me, though I tend to agree with mat@ about removing the two cases of "...will be added to..."
debugging.

What are the next steps? I can commit this (and make the change myself), or does portmgr@ want to review
or possibly do an -exp run?

[gerald]

Ah, one thing I nearly forgot: Since you kept USE_GCC=run in the
updated patch, I assume there is at least one tree in the port that
can leverage this after your patch?

I did not mean to contradict mat@ with my approval.

Sorry about not stating that in my original note a few minutes ago.

[gerald]

As a bit of background, that small infrastruture improvement got
stalled (by one port at this point), and I simply did not want to
make this delay any further, so putting this on the fast lane,
finally.

[tobik]

In D7223#218527, @gerald wrote:

As a bit of background, that small infrastruture improvement got
stalled (by one port at this point), and I simply did not want to
make this delay any further, so putting this on the fast lane,
finally.

Is there any reason this needs to be delayed any longer?

[tobik]

Can we do something with this?

Mk/bsd.gcc.mk

89 ↗

(On Diff #26422)

This should probably use IGNORE=.

[linimon]

Mk/bsd.gcc.mk

89 ↗

(On Diff #26422)

Please, please, please. Use of .error breaks metadata sweeps (such as when you build INDEX).

[gerald]

Mk/bsd.gcc.mk

89 ↗

(On Diff #26422)

Yep, changed.

254–262 ↗

(On Diff #18747)

I agree with Mat, though it's even stronger: note how just a few lines
below the already existing debugging output prints BUILD_DEPENDS
and RUN_DEPENDS. So this is largely unnecessary.

[gerald]

In D7223#430290, @tobik wrote:

Can we do something with this?

Yep, I've got an updated/simplified patch ready.

And an update of your net/ipxe port as an example as part of that. :-)

[fabian.freyer_physik.tu-berlin.de]

Great, thanks @gerald! And sorry for letting this gather dust!

[gerald]

Okay, I am getting officially desparate. :-(

How can I upload a new diff for this issue in this tool? "Update diff..." always seems to want to create a new issue instead of appending to this one, regardless of web browser (even my "accept all trackers and cookies" one).

So here we go, the updated diff below; feedback very welcome.

Index: Mk/bsd.gcc.mk

• Mk/bsd.gcc.mk (revision 501438)

+++ Mk/bsd.gcc.mk (working copy)
@@ -17,6 +17,15 @@

1. do so by specifying USE_GCC=X+ which requests at least GCC version X.
2. To request a specific version omit the trailing + sign. #

+# Optional comma-separated arguments may be specified after the version
+# specifier. The following arguments are supported:
+# build ... Add GCC as a build dependency (BUILD_DEPENDS).
+# If no arguments are specified, GCC is added both as a build dependency
+# and a runtime dependency.
+#
+# USE_GCC=build is a shortcut for USE_GCC=yes:build.
+#
+#

1. Examples:
2. USE_GCC= yes # port requires a current version of GCC
3. # as defined in bsd.default-versions.mk.

@@ -23,6 +32,9 @@

1. USE_GCC= any # port requires GCC 4.2 or later.
2. USE_GCC= 7+ # port requires GCC 7 or later.
3. USE_GCC= 5 # port requires GCC 5.

+# USE_GCC= 8:build # port requires GCC 8 at build-time only.
+# USE_GCC= build # port requires a current version of GCC at
+# # build-time only.

2. If you are wondering what your port exactly does, use "make test-gcc"
3. to see some debugging.

@@ -49,6 +61,35 @@

1. No configurable parts below this. ######## #

+# Alias USE_GCC=build to USE_GCC=yes:build
+.if ${USE_GCC} == build
+USE_GCC:= yes:${USE_GCC}
+.endif
+
+# Split arguments
+.if defined(USE_GCC)
+USE_GCC:= ${USE_GCC:C/\:.*}
+_USE_GCC_ARGS:= ${USE_GCC:C/^[^\:]*(\:|\$):S/,/ /g}
+USE_GCC= ${USE_GCC}
+.endif
+
+.if ${_USE_GCC_ARGS:Mbuild}
+_USE_GCC_BUILD_DEP= yes
+_USE_GCC_ARGS:= ${_USE_GCC_ARGS:Nbuild}
+.endif
+
+.if !empty(_USE_GCC_ARGS)
+IGNORE= Bad target specification in USE_GCC; only "build" is supported
+.endif
+
+# If the port does not specify a specific dependency, assume all are required.
+.if !defined(_USE_GCC_BUILD_DEP) && !defined(_USE_GCC_RUN_DEP)
+_USE_GCC_BUILD_DEP:= yes
+_USE_GCC_RUN_DEP:= yes
+.endif
+
+
+# Handle USE_GCC=yes.
.if defined(USE_GCC) && ${USE_GCC} == yes
USE_GCC= ${GCC_DEFAULT}+
.endif
@@ -177,8 +218,12 @@
CXXFLAGS:= ${CXXFLAGS:N-mretpoline}

.if defined(_GCC_PORT_DEPENDS)
+. if defined(_USE_GCC_BUILD_DEP)
BUILD_DEPENDS+= ${_GCC_PORT_DEPENDS}:lang/${_GCC_PORT}
+. endif
+. if defined(_USE_GCC_RUN_DEP)
RUN_DEPENDS+= ${_GCC_PORT_DEPENDS}:lang/${_GCC_PORT}
+. endif

1. Later GCC ports already depend on binutils; make sure whatever we
2. build leverages this as well. USE_BINUTILS= yes

[gerald]

I made all changes suggested plus simplified to only cover what I saw as required by the current ports tree (plus minor simplifications/updates)

[tobik]

In D7223#436394, @gerald wrote:

I made all changes suggested plus simplified to only cover what I saw as required by the current ports tree (plus minor simplifications/updates)

This looks fine to me (i.e., I do not see anything really wrong and I can still build net/ipxe).

In D7223#436391, @gerald wrote:

Okay, I am getting officially desparate. :-(

How can I upload a new diff for this issue in this tool? "Update diff..." always seems to want to create a new issue instead of appending to this one, regardless of web browser (even my "accept all trackers and cookies" one).

I can only recommend to install devel/arcanist and setting it up according to https://wiki.freebsd.org/Phabricator#Configure_Client. Then updating a review becomes just arc diff --update D7223. It also generates a diff with full context automatically (something this review lacks at the moment) which can make reviews easier.

Mk/bsd.gcc.mk

82 ↗

(On Diff #57354)

Maybe start it with has or any other verb so that it does not read weird like

===>  ipxe-20190307 Bad target specification in USE_GCC; only "build" is
supported.

Though the framework is hardly consistent here.

[linimon]

Is there anything else that needs to be changed before this goes in? Is it still even relevant?

[gerald]

Further simplifications and some updates, focusing on USE_GCC=...:build only, which is the only case we need in the Ports Collection.