Re: Expanding the default OPSYSVARS

["Darrin B. Jewell" <dbj%netbsd.org@localhost>]

Alistair Crooks <agc%pkgsrc.org@localhost> writes:

> Hi Jonathan,
>
> I'd like us to do this properly, and we're very near the start of the
> freeze - as you note, there are a number of these changes, so
> depending on the successful results of your bulk build, please
> consider this a go-ahead for making the OPSYSVARS for PKG_OPTIONS
> infrastructure change during the freeze.
>
> Thanks,
> Alistair
>
> On 18 March 2016 at 02:46, Jonathan Perkin <jperkin%joyent.com@localhost> wrote:
>> * On 2016-03-18 at 04:07 GMT, Darrin B. Jewell wrote:
>>
>>> Comment below about PKG_SUGGESTED_OPTIONS.${OPSYS} not working correctly below.
>>>
>>> David Holland <dholland-pkgtech%netbsd.org@localhost> writes:
>>>
>>> > On Mon, Feb 22, 2016 at 03:24:31PM +0000, Jonathan Perkin wrote:
>>> >  > with 'SUBST_CLASSES' listed in OPSYSVARS, one only needs to do this:
>>> >  >
>>> >  >   SUBST_CLASSES.Darwin+= foo
>>> >  >
>>> >  > I think one of the reasons it gets so little use is that the default
>>> >  > list of variables is too short.  I'd like to add the following
>>> >  > variables to the default list, based on some quick analysis of the
>>> >  > most-used variables (those used >30 times) within .if ${OPSYS} blocks:
>>> >  >
>>> >  >   BUILD_TARGET
>>> >  >   BUILDLINK_TRANSFORM
>>> >  >   CONFIGURE_ARGS
>>> >  >   CONFIGURE_ENV
>>> >  >   MAKE_ENV
>>> >  >   PKG_SUGGESTED_OPTIONS
>>> >  >   SUBST_CLASSES
>>> >  >   USE_TOOLS
>>> >
>>> > yes please :-)
>>> >
>>> > I'd add PKG_SUPPORTED_OPTIONS to that list - doesn't appear that often
>>> > but when it does parallelism with PKG_SUGGESTED_OPTIONS will improve
>>> > clarity.
>>> >
>>>
>>> Unfortunately, PKG_SUGGESTED_OPTIONS isn't working correctly as an OPSYSVAR
>>> A value placed in PKG_SUGGESTED_OPTIONS.Darwin for example does end up
>>> in PKG_SUGGESTED_OPTIONS, but it doesn't get correctly propagated to PKG_OPTIONS
>>> so the packages don't actually pick up the option.
>>>
>>> I'm not sure what the right fix is.
>>
>> Sorry, I thought I'd tested this pretty thoroughly but obviously
>> missed the fact that we need the variables evaluated before we can
>> check PKG_OPTIONS, so lots is broken by this.  Can you try:
>>
>>   https://github.com/joyent/pkgsrc/commit/18e6b1a
>>
>> This seems to work fine for my test package (devel/boehm-gc on
>> Darwin), and I'm pushing this through a bulk build test too.

I've tested this fix.  It works.  However my test is the same as yours
(discovered via www/w3m breakage).

There is a minor nit with your fix that it occurs after the check for
PKG_OPTIONS_VAR so that a package that defines a complete set of
PKG_SUPPORTED_OPTIONS.${OPSYS} for all platforms but not
PKG_SUPPORTED_OPTIONS itself may fail.  However, I'm sure that's a
corner case we won't hit.

Since I sent this original email, Taylor Campbell suggested "An easy
way to fix would be to move the OPSYSVARS stuff to another file
opsysvars.mk with include protection which both bsd.options.mk and
bsd.pkg.mk include."

So I have attached such a patch to this email.
I'm ok with either fix.

Thanks for your attention to this.

Darrin

Index: pkgsrc/mk/bsd.pkg.mk
===================================================================
RCS file: /cvsroot/pkgsrc/mk/bsd.pkg.mk,v
retrieving revision 1.2017
diff -u -r1.2017 bsd.pkg.mk
--- pkgsrc/mk/bsd.pkg.mk	26 Feb 2016 11:38:37 -0000	1.2017
+++ pkgsrc/mk/bsd.pkg.mk	19 Mar 2016 05:08:44 -0000
@@ -137,19 +137,7 @@
 PKG_FAIL_REASON+='Please unset PKG_PATH before doing pkgsrc work!'
 .endif
 
-# Allow variables to be set on a per-OS basis
-OPSYSVARS+=	CFLAGS CXXFLAGS CPPFLAGS LDFLAGS LIBS
-OPSYSVARS+=	CMAKE_ARGS CONFIGURE_ARGS CONFIGURE_ENV
-OPSYSVARS+=	BUILDLINK_TRANSFORM SUBST_CLASSES
-OPSYSVARS+=	BUILD_TARGET MAKE_ENV MAKE_FLAGS USE_TOOLS
-OPSYSVARS+=	PKG_SUPPORTED_OPTIONS PKG_SUGGESTED_OPTIONS
-.for _var_ in ${OPSYSVARS:O}
-.  if defined(${_var_}.${OPSYS})
-${_var_}+=	${${_var_}.${OPSYS}}
-.  elif defined(${_var_}.*)
-${_var_}+=	${${_var_}.*}
-.  endif
-.endfor
+.include "opsysvars.mk"
 
 CPPFLAGS+=	${CPP_PRECOMP_FLAGS}
 
Index: pkgsrc/mk/bsd.options.mk
===================================================================
RCS file: /cvsroot/pkgsrc/mk/bsd.options.mk,v
retrieving revision 1.71
diff -u -r1.71 bsd.options.mk
--- pkgsrc/mk/bsd.options.mk	7 Jun 2013 00:41:39 -0000	1.71
+++ pkgsrc/mk/bsd.options.mk	19 Mar 2016 05:08:44 -0000
@@ -161,6 +161,7 @@
 _SYS_VARS.options=	PKG_OPTIONS
 
 .include "bsd.prefs.mk"
+.include "opsysvars.mk"
 
 # Define PKG_OPTIONS, no matter if we have an error or not, to suppress
 # further make(1) warnings.
Index: pkgsrc/mk/opsysvars.mk
===================================================================
RCS file: pkgsrc/mk/opsysvars.mk
diff -N pkgsrc/mk/opsysvars.mk
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ pkgsrc/mk/opsysvars.mk	19 Mar 2016 05:08:44 -0000
@@ -0,0 +1,20 @@
+# $NetBSD: $
+
+.if !defined(OPSYSVARS_MK)
+OPSYSVARS_MK=	# defined
+
+# Allow variables to be set on a per-OS basis
+OPSYSVARS+=	CFLAGS CXXFLAGS CPPFLAGS LDFLAGS LIBS
+OPSYSVARS+=	CMAKE_ARGS CONFIGURE_ARGS CONFIGURE_ENV
+OPSYSVARS+=	BUILDLINK_TRANSFORM SUBST_CLASSES
+OPSYSVARS+=	BUILD_TARGET MAKE_ENV MAKE_FLAGS USE_TOOLS
+OPSYSVARS+=	PKG_SUPPORTED_OPTIONS PKG_SUGGESTED_OPTIONS
+.for _var_ in ${OPSYSVARS:O}
+.  if defined(${_var_}.${OPSYS})
+${_var_}+=	${${_var_}.${OPSYS}}
+.  elif defined(${_var_}.*)
+${_var_}+=	${${_var_}.*}
+.  endif
+.endfor
+
+.endif	# OPSYSVARS_MK

>>
>> Alistair, heads up that we may need this during the freeze
>> unfortunately :(  Alternatively we back out any changes to convert
>> PKG_*OPTIONS to OPSYSVARS, though there are quite a few.
>>
>> --
>> Jonathan Perkin  -  Joyent, Inc.  -  www.joyent.com
>>