Like most people, I have my pet peeves. I consider that documentation
is very important to code quality.
I ran into a school-book example a week ago, so I decided to share what I
mean.
There was this problem with dpb where some packages in a MULTI_PACKAGES show
up as Errors when they don't build on a given architecture, even though
they're marked as NOT_FOR_ARCHS-sub = ...
I had fixed that particular way of building things by getting those to
vanish when building all packages, so that the build would not error out,
but dpb was still getting them as errors (and the remaining subpackages were
built correclty)
Looking more closely, dpb actually dumps information for all subpackages,
including the non-building ones. Not including the non-building ones would
not work, because the listing job would retry and retry. Nope, what was needed
was the same mechanism that lets dpb ignore regular packages, that is the
IGNORE variable.
So, all that was needed was to set the IGNORE variable on a subpackage
dependent basis, and dpb would happily trudge along.
Thus, a patch was born, tested and committed that would make IGNORE subpackage
dependent. I tried to make it as simple as could be:
(the relevant part is setting all
IGNORE-sub, then having
IGNORE = ${IGNORE-sub} and testing IGNORE,
plus marking IGNORE as subpackage dependent for dump-vars)
Index: bsd.port.mk
===================================================================
RCS file: /home/openbsd/cvs/ports/infrastructure/mk/bsd.port.mk,v
retrieving revision 1.1074
retrieving revision 1.1075
diff -u -p -r1.1074 -r1.1075
--- bsd.port.mk 28 Mar 2011 00:16:13 -0000 1.1074
+++ bsd.port.mk 3 Apr 2011 07:19:05 -0000 1.1075
@@ -92,11 +92,11 @@ _SHSCRIPT = sh ${PORTSDIR}/infrastructur
_PERLSCRIPT = perl ${PORTSDIR}/infrastructure/bin
# All variables relevant to the port's description
-_ALL_VARIABLES = BUILD_DEPENDS IGNORE IS_INTERACTIVE \
+_ALL_VARIABLES = BUILD_DEPENDS IS_INTERACTIVE \
SUBPACKAGE MULTI_PACKAGES
# and stuff needing to be MULTI_PACKAGE'd
_ALL_VARIABLES_INDEXED = FULLPKGNAME RUN_DEPENDS LIB_DEPENDS \
- PKG_ARCH
+ PKG_ARCH IGNORE
_ALL_VARIABLES_PER_ARCH =
.if ${DPB:L:Mfetch}
@@ -1320,7 +1320,6 @@ IS_INTERACTIVE = Yes
#
# Don't build a port if it comes with the base system.
################################################################
-IGNORE ?=
TRY_BROKEN ?= No
_IGNORE_REGRESS ?=
.if defined(REGRESS_IS_INTERACTIVE) && defined(BATCH)
@@ -1328,44 +1327,49 @@ _IGNORE_REGRESS += "has interactive test
.elif !defined(REGRESS_IS_INTERACTIVE) && defined(INTERACTIVE)
_IGNORE_REGRESS += "does not have interactive tests"
.endif
-.if defined(IS_INTERACTIVE) && defined(BATCH)
-IGNORE += "is an interactive port"
-.elif !defined(IS_INTERACTIVE) && defined(INTERACTIVE)
-IGNORE += "is not an interactive port"
-.endif
-.if !exists(${X11BASE})
-IGNORE += "building ports requires X11 but ${X11BASE} not found"
-.endif
-.if !defined(_ARCH_OK${SUBPACKAGE}) || ${_ARCH_OK${SUBPACKAGE}} == 0
-. if defined(ONLY_FOR_ARCHS${SUBPACKAGE})
-. if ${MACHINE_ARCH} == "${ARCH}"
-IGNORE += "is only for ${ONLY_FOR_ARCHS${SUBPACKAGE}}, not ${MACHINE_ARCH}"
+
+.for _s in ${MULTI_PACKAGES}
+IGNORE${_s} ?=
+. if defined(IS_INTERACTIVE) && defined(BATCH)
+IGNORE${_s} += "is an interactive port"
+. elif !defined(IS_INTERACTIVE) && defined(INTERACTIVE)
+IGNORE${_s} += "is not an interactive port"
+. endif
+. if !exists(${X11BASE})
+IGNORE${_s} += "building ports requires X11 but ${X11BASE} not found"
+. endif
+. if !defined(_ARCH_OK${_s}) || ${_ARCH_OK${_s}} == 0
+. if defined(ONLY_FOR_ARCHS${_s})
+. if ${MACHINE_ARCH} == "${ARCH}"
+IGNORE${_s} += "is only for ${ONLY_FOR_ARCHS${_s}}, not ${MACHINE_ARCH}"
+. else
+IGNORE${_s} += "is only for ${ONLY_FOR_ARCHS${_s}}, not ${MACHINE_ARCH}
\(${ARCH}\)"
+. endif
. else
-IGNORE += "is only for ${ONLY_FOR_ARCHS${SUBPACKAGE}}, not ${MACHINE_ARCH}
\(${ARCH}\)"
+IGNORE${_s} += "is not for ${NOT_FOR_ARCHS}"
. endif
-. else
-IGNORE += "is not for ${NOT_FOR_ARCHS}"
. endif
-.endif
-.if ${SHARED_ONLY:L} == "yes" && ${NO_SHARED_LIBS:L} == "yes"
-IGNORE += "requires shared libraries"
-.endif
-
-.if ${TRY_BROKEN:L} != "yes"
-. if defined(BROKEN-${ARCH})
-IGNORE += "is marked as broken for ${ARCH}: ${BROKEN-${ARCH}:Q}"
+. if ${SHARED_ONLY:L} == "yes" && ${NO_SHARED_LIBS:L} == "yes"
+IGNORE${_s} += "requires shared libraries"
. endif
-. if ${MACHINE_ARCH} != ${ARCH} && defined(BROKEN-${MACHINE_ARCH})
-IGNORE += "is marked as broken for ${MACHINE_ARCH}:
${BROKEN-${MACHINE_ARCH}:Q}"
+
+. if ${TRY_BROKEN:L} != "yes"
+. if defined(BROKEN-${ARCH})
+IGNORE${_s} += "is marked as broken for ${ARCH}: ${BROKEN-${ARCH}:Q}"
+. endif
+. if ${MACHINE_ARCH} != ${ARCH} && defined(BROKEN-${MACHINE_ARCH})
+IGNORE${_s} += "is marked as broken for ${MACHINE_ARCH}:
${BROKEN-${MACHINE_ARCH}:Q}"
+. endif
+. if defined(BROKEN)
+IGNORE${_s} += "is marked as broken: ${BROKEN:Q}"
+. endif
. endif
-. if defined(BROKEN)
-IGNORE += "is marked as broken: ${BROKEN:Q}"
+. if defined(COMES_WITH)
+IGNORE${_s} += "-- ${FULLPKGNAME${SUBPACKAGE}:C/-[0-9].*//g} comes with
OpenBSD as of release ${COMES_WITH}"
. endif
-.endif
-.if defined(COMES_WITH)
-IGNORE += "-- ${FULLPKGNAME${SUBPACKAGE}:C/-[0-9].*//g} comes with OpenBSD as
of release ${COMES_WITH}"
-.endif
+.endfor
+IGNORE = ${IGNORE${SUBPACKAGE}}
IGNORE_IS_FATAL ?= "No"
.if !empty(IGNORE) && ${IGNORE_IS_FATAL:L} == "yes"
ERRORS += "Fatal: can't build"
So all was fine. All was fine ? Well, I wrote the documentation part.
Yeouch. Now, new IGNORE was not behaving at all like the other indexed
variables. It did work, but it was a very special case...
So I sat down again, and wrote a new version of the bsd.port.mk patch.
Turns out there was a nicer, more regular way to do things.
Now, instead of defining IGNORE-sub backwards, I do derive IGNORE-sub
from normal IGNORE. The only -subpackage dependent part in bsd.port.mk
concerns adding ONLY_FOR_ARCHS/NOT_FOR_ARCHS to the right variable.
Not too surprisingly, some stuff vanishes (namely __ARCH_OK-sub is only
used to set IGNORE-sub, so let's set IGNORE-sub directly).
Looking more closely at all uses of IGNORE* and ONLY_FOR_ARCHS*, I also
fixed a bug later on with respect to describe (which was written a looong
time ago and which is obviously not interpreting the difference between
empty ONLY_FOR_ARCHS and undefined ONLY_FOR_ARCHS).
(to be committed once it's fully tested. There may be typos in there).
Index: bsd.port.mk
===================================================================
RCS file: /home/openbsd/cvs/ports/infrastructure/mk/bsd.port.mk,v
retrieving revision 1.1074
diff -u -p -r1.1074 bsd.port.mk
--- bsd.port.mk 28 Mar 2011 00:16:13 -0000 1.1074
+++ bsd.port.mk 8 Apr 2011 21:10:39 -0000
@@ -92,11 +92,11 @@ _SHSCRIPT = sh ${PORTSDIR}/infrastructur
_PERLSCRIPT = perl ${PORTSDIR}/infrastructure/bin
# All variables relevant to the port's description
-_ALL_VARIABLES = BUILD_DEPENDS IGNORE IS_INTERACTIVE \
+_ALL_VARIABLES = BUILD_DEPENDS IS_INTERACTIVE \
SUBPACKAGE MULTI_PACKAGES
# and stuff needing to be MULTI_PACKAGE'd
_ALL_VARIABLES_INDEXED = FULLPKGNAME RUN_DEPENDS LIB_DEPENDS \
- PKG_ARCH
+ PKG_ARCH IGNORE
_ALL_VARIABLES_PER_ARCH =
.if ${DPB:L:Mfetch}
@@ -441,28 +441,36 @@ ONLY_FOR_ARCHS${_s} ?= ${ONLY_FOR_ARCHS}
NOT_FOR_ARCHS${_s} ?= ${NOT_FOR_ARCHS}
. endif
+IGNORE${_s} ?=
+IGNORE${_s} += ${IGNORE}
+
# compute _ARCH_OK for ignore
. if defined(ONLY_FOR_ARCHS${_s})
-_ARCH_OK${_s} = 0
+_ARCH_OK = 0
. for __ARCH in ${MACHINE_ARCH} ${ARCH}
. if !empty(ONLY_FOR_ARCHS${_s}:M${__ARCH})
-_ARCH_OK${_s} = 1
+_ARCH_OK = 1
. endif
. endfor
-. else
-_ARCH_OK${_s} = 1
+. if ${_ARCH_OK} == 0
+. if ${MACHINE_ARCH} == "${ARCH}"
+IGNORE${_s} += "is only for ${ONLY_FOR_ARCHS${_s}}, not ${MACHINE_ARCH}"
+. else
+IGNORE${_s} += "is only for ${ONLY_FOR_ARCHS${_s}}, not ${MACHINE_ARCH}
\(${ARCH}\)"
+. endif
+. endif
. endif
. if defined(NOT_FOR_ARCHS${_s})
. for __ARCH in ${MACHINE_ARCH} ${ARCH}
. if !empty(NOT_FOR_ARCHS${_s}:M${__ARCH})
-_ARCH_OK${_s} = 0
+IGNORE${_s} += "is not for ${NOT_FOR_ARCHS${_s}}"
. endif
. endfor
. endif
# allow subpackages to vanish on architectures that don't
# support them
-. if ${_ARCH_OK${_s}} == 1
+. if empty(IGNORE${_s})
_MULTI_PACKAGES += ${_s}
. endif
.endfor
@@ -1320,7 +1328,6 @@ IS_INTERACTIVE = Yes
#
# Don't build a port if it comes with the base system.
################################################################
-IGNORE ?=
TRY_BROKEN ?= No
_IGNORE_REGRESS ?=
.if defined(REGRESS_IS_INTERACTIVE) && defined(BATCH)
@@ -1328,6 +1335,7 @@ _IGNORE_REGRESS += "has interactive test
.elif !defined(REGRESS_IS_INTERACTIVE) && defined(INTERACTIVE)
_IGNORE_REGRESS += "does not have interactive tests"
.endif
+
.if defined(IS_INTERACTIVE) && defined(BATCH)
IGNORE += "is an interactive port"
.elif !defined(IS_INTERACTIVE) && defined(INTERACTIVE)
@@ -1336,17 +1344,7 @@ IGNORE += "is not an interactive port"
.if !exists(${X11BASE})
IGNORE += "building ports requires X11 but ${X11BASE} not found"
.endif
-.if !defined(_ARCH_OK${SUBPACKAGE}) || ${_ARCH_OK${SUBPACKAGE}} == 0
-. if defined(ONLY_FOR_ARCHS${SUBPACKAGE})
-. if ${MACHINE_ARCH} == "${ARCH}"
-IGNORE += "is only for ${ONLY_FOR_ARCHS${SUBPACKAGE}}, not ${MACHINE_ARCH}"
-. else
-IGNORE += "is only for ${ONLY_FOR_ARCHS${SUBPACKAGE}}, not ${MACHINE_ARCH}
\(${ARCH}\)"
-. endif
-. else
-IGNORE += "is not for ${NOT_FOR_ARCHS}"
-. endif
-.endif
+
.if ${SHARED_ONLY:L} == "yes" && ${NO_SHARED_LIBS:L} == "yes"
IGNORE += "requires shared libraries"
.endif
@@ -1367,9 +1365,9 @@ IGNORE += "-- ${FULLPKGNAME${SUBPACKAGE}
.endif
IGNORE_IS_FATAL ?= "No"
-.if !empty(IGNORE) && ${IGNORE_IS_FATAL:L} == "yes"
+.if !empty(IGNORE${SUBPACKAGE}) && ${IGNORE_IS_FATAL:L} == "yes"
ERRORS += "Fatal: can't build"
-ERRORS += ${IGNORE}
+ERRORS += ${IGNORE${SUBPACKAGE}}
.endif
.if !defined(DEPENDS_TARGET)
@@ -2024,7 +2022,7 @@ _internal-fetch-all:
@${_MAKE} post-fetch __FETCH_ALL=Yes
.endif
-.if !empty(IGNORE) && !defined(NO_IGNORE)
+.if !empty(IGNORE${SUBPACKAGE}) && !defined(NO_IGNORE)
_internal-all _internal-build _internal-checksum _internal-configure \
_internal-deinstall _internal-extract _internal-fake _internal-fetch \
_internal-install _internal-install-all _internal-manpages-check \
@@ -2034,10 +2032,10 @@ _internal-all _internal-build _internal-
_internal-update-or-install-all _internal-update-plist \
port-lib-depends-check update-patches:
. if !defined(IGNORE_SILENT)
- @${ECHO_MSG} "===> ${FULLPKGNAME${SUBPACKAGE}}${_MASTER} ${IGNORE}."
+ @${ECHO_MSG} "===> ${FULLPKGNAME${SUBPACKAGE}}${_MASTER}
${IGNORE${SUBPACKAGE}}."
. endif
. if defined(_IGNORE_COOKIE)
- @echo "${IGNORE}" >${_IGNORE_COOKIE}
+ @echo "${IGNORE${SUBPACKAGE}}" >${_IGNORE_COOKIE}
. endif
.else
@@ -2908,14 +2906,13 @@ describe:
@echo -n '${_${_d}_DEP3${_S}:C/ +/ /g}'| tr '\040' '\012'|sort -u|tr
'\012' '\040' | sed -e 's, $$,,'
@echo -n '|'
. endfor
- @case "${ONLY_FOR_ARCHS}" in \
- "") case "${NOT_FOR_ARCHS}" in \
- "") echo -n "any|";; \
- *) echo -n "!${NOT_FOR_ARCHS}|";; \
- esac;; \
- *) echo -n "${ONLY_FOR_ARCHS}|";; \
- esac
-
+. if defined(ONLY_FOR_ARCHS${_S})
+ @echo -n "${ONLY_FOR_ARCHS${_S}}|"
+. elif defined(NOT_FOR_ARCHS${_S})
+ @echo -n "!${NOT_FOR_ARCHS${_S}}|"
+. else
+ @echo -n "any|"
+. endif
. if defined(_BAD_LICENSING)
@echo "?|?|?|?"
. else
And now, the documentation writes itself... or rather, things are very
simple to explain, and quite sane compared to the first version.
This is just a small example. That shit happens to me all the time.
Specifically, bsd.port.mk got improved immensely those past few years, based
on actual usage, based on taking feedback from fellow porters at the ports
hackathons (old-timers will recall current MULTI_PACKAGES looks nothings like
the first versions of it), and also based on documenting stuff.
I almost never document stuff before writing the code. I have an idea of
what I want to do (informal spec), then I write and test the code, then
I validate what I did by documenting it. And most of the time, I find obvious
improvements just by doing that: documentation forces me to look at things
another way, and to make things better.
