Page MenuHomeFreeBSD

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

Authored by gerald on Jul 16 2016, 1:50 PM.

Details

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.

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

fabian.freyer_physik.tu-berlin.de retitled this revision from to add support for conditional dependencies for USE_GCC.Jul 16 2016, 1:50 PM
fabian.freyer_physik.tu-berlin.de edited the test plan for this revision. (Show Details)
fabian.freyer_physik.tu-berlin.de set the repository for this revision to rP FreeBSD ports repository.
mat added a comment.Jul 19 2016, 3:37 PM

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

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?

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

koobs added a subscriber: koobs.Jul 21 2016, 11:39 AM
gerald edited edge metadata.Jul 25 2016, 7:34 PM
gerald requested changes to this revision.

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?

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

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

  1. In the comments, please refer to "GCC" (uppercase), not "gcc".
This revision now requires changes to proceed.Jul 25 2016, 7:34 PM

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.

  1. 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/

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

No problem here.

  • used GCC instead of gcc in comments
  • only add to RUN_DEPENDS and BUILD_DEPENDS by default.
koobs added a comment.Jul 26 2016, 5:20 AM

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 added a comment.Jul 26 2016, 8:24 AM

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

This looks like debugging info, it should be removed.

254–262

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

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

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

see above.

mat added inline comments.Jul 26 2016, 11:26 AM
Mk/bsd.gcc.mk
246–248

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 :-)

  • Added error handling
  • Rebased onto r436066
  • Added full context
mat added a comment.Mar 13 2017, 5:00 PM

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.

dch added a subscriber: dch.Mar 16 2017, 12:59 PM

Remove TEST_DEPENDS stuff.

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

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 accepted this revision.Apr 30 2017, 10:34 AM

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?

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.

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.

swills added a subscriber: swills.Nov 21 2017, 4:13 PM
tobik added a subscriber: tobik.Dec 12 2017, 11:24 AM

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?

linimon retitled this revision from add support for conditional dependencies for USE_GCC to Mk/bsd.gcc.mk: add support for conditional dependencies for USE_GCC.Jul 18 2018, 1:00 PM
tobik added a comment.Apr 23 2019, 5:23 AM

Can we do something with this?

Mk/bsd.gcc.mk
89

This should probably use IGNORE=.

linimon added inline comments.Apr 23 2019, 5:35 AM
Mk/bsd.gcc.mk
89

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

gerald added inline comments.May 9 2019, 8:51 PM
Mk/bsd.gcc.mk
89

Yep, changed.

254–262

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 added a comment.May 9 2019, 9:03 PM
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. :-)

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

gerald commandeered this revision.

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.

  1. If you are wondering what your port exactly does, use "make test-gcc"
  2. 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 updated this revision to Diff 57354.May 13 2019, 6:53 AM

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

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).

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

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.

tobik removed a subscriber: tobik.Jun 2 2019, 1:48 PM