Re: [patch] Test target PATH
I have to agree with Stuart there. Changing stuff in there might cause problems down the road. Also note that stuff like the install-wrapper might get picked up by configure and such... so changing the PATH in a single port is not enough in general. I think an actual fix would involve making the wrappers aware of TEST mode and remove their "I'm not touching anything" behavior in such a case. Not too fond of doing that because of the extra complexity, but that would be the correct fix. Right now, I'm more of a "let's just fix the zsh tests".
Re: [patch] Test target PATH
On Fri, Aug 28, 2015 at 08:47:05PM +0100, Stuart Henderson wrote:
On 2015/08/27 22:04, Matthew Martin wrote:
${WRKDIR}/bin is only in PATH during extract, configure, build, fake,
and test. Since fake creates the chown and chgrp symlinks, extract,
configure, build, and test must not depend on those symlinks as they
don't depend on fake. Would it be acceptable to clean up the symlinks
after fake completes or rm -f them in test in case test is run after
fake? Both patches below fix the issue.
However, I still believe that the first patch is correct as any ports it
breaks has tests that depend on non-standard install(1) behavior caused
by the perl wrapper so including ${WRKDIR}/bin in PATH is just
concealing the breakage.
This directory is used for other things which may get called during
test, for example symlinks bin/python - /usr/local/bin/python-2.7 or
bin/cc - /usr/local/bin/egcc, so I think that the patch which removes
${WRKDIR}/bin from PATH during test is incorrect.
Fair enough.
+ @rm -f ${WRKDIR}/bin/chown ${WRKDIR}/bin/chgrp
If you're going to do this, wouldn't it make sense to rm the install
wrapper as well?
I didn't rm the install wrapper because Juan raised concerns about
regressions. Since both chown and chgrp are created by fake, there
couldn't be any test regressions this way. Although as I noted any test
that relies on the install wrapper is likely broken anyway.
There might be a problem though, I suspect it will break things with
SEPARATE_BUILD=flavored, so from that perspective it would be safer
to target it tightly in the port which is actually affected by it
even if it's not quite correct.
I'm not familiar with how SEPARATE_BUILD works, so I'll defer to you on
this. While I'd prefer a fix in the ports system, if it'd have other
fallout, a patch for zsh is fine.
- Matthew Martin
Re: [patch] Test target PATH
On Thu, Aug 27, 2015 at 11:27:44PM -0400, Raul Miller wrote:
On Thu, Aug 27, 2015 at 11:04 PM, Matthew Martin phy1...@gmail.com wrote:
On Thu, Aug 27, 2015 at 11:47:19PM +0200, Juan Francisco Cantero Hurtado
wrote:
---rm -f ${WRKDIR}/bin/chgrp
While this would fix the problem, since the problem is not in zsh
itself, I don't think that the zsh port is the right place to fix this.
Since the bug only occurs in the port of zsh, that means that the zsh
port is the right place to fix this.
Just because I found the bug while messing with the zsh port, doesn't
mean that there aren't other ports that are broken in the same way. Any
port that needs chown or chgrp during its test is similarly broken.
Or am I missing some important issue? (If so, could you explain what breaks?)
I thought I had explained what breaks, but once more can't hurt.
Test C02cond.ztst contains the following among others
touch zerolength
chgrp $EGID zerolength
[...]
[[ -G zerolength ]]
touch zerolength creates a file with uid:gid 1000:0 (assuming your user
is uid 1000). chgrp then changes the gid of zerolength to 1000 as that's
my primary gid. The last line tests that the file zerolength is owned by
the effecive gid of the shell.
For a typical make test, fake is not run (since test only depends on
build and most people remember to run test before fake) and the test
succeeds. For make fake test (or running test anytime after fake) the
test fails. This is because the test target (among other things) runs
the tests with
@cd ${WRKBUILD} exec 31 exit `exec 41 13; \
(exec; set +e; ${SETENV} ${ALL_TEST_ENV} ${MAKE_PROGRAM} \
${ALL_TEST_FLAGS} -f ${MAKE_FILE} ${TEST_TARGET}; \
echo $$? 4) 21 ${TEST_LOG}`
The PATH is containted in ALL_TEST_ENV which is defined as
ALL_TEST_ENV = ${MAKE_ENV} ${TEST_ENV}
TEST_ENV is typically blank and the environment comes from MAKE_ENV
MAKE_ENV += PATH='${PORTPATH}' PREFIX='${PREFIX}' \
LOCALBASE='${LOCALBASE}' DEPBASE='${DEPBASE}' X11BASE='${X11BASE}' \
CFLAGS='${CFLAGS:C/ *$//}' \
TRUEPREFIX='${PREFIX}' ${DESTDIRNAME}='' \
HOME='${PORTHOME}'
defined thusly. PATH is equal to PORTPATH which a few lines above is
PORTPATH ?=
${WRKDIR}/bin:/usr/bin:/bin:/usr/sbin:/sbin:${DEPBASE}/bin:${LOCALBASE}/bin:${X11BASE}/bin
Now when `chgrp ...` is run we first search in ${WRKDIR}/bin for chgrp.
Typically this directory does not contain a chgrp and the shell
continues down the PATH to find /bin/chgrp; however, the fake target has
@ln -sf /bin/echo ${WRKDIR}/bin/chown
@ln -sf /bin/echo ${WRKDIR}/bin/chgrp
run whenever ${FAKE_AS_ROOT:L} != yes. fake does not rm these links
after it has finished. Thus anytime that tests are run after fake has
run, chown and chgrp are just echo and do nothing.
Since chgrp does nothing in this case and 0 != 1000, [[ -G zerolength ]]
fails as does the test. Hopefully it's now obvious that zsh expecting
for chgrp to actually change groups is not a bug in zsh and that chgrp
failing to change a file's group while tests are being run is a bug in
the ports system.
- Matthew Martin
Re: [patch] Test target PATH
On 2015/08/27 22:04, Matthew Martin wrote:
${WRKDIR}/bin is only in PATH during extract, configure, build, fake,
and test. Since fake creates the chown and chgrp symlinks, extract,
configure, build, and test must not depend on those symlinks as they
don't depend on fake. Would it be acceptable to clean up the symlinks
after fake completes or rm -f them in test in case test is run after
fake? Both patches below fix the issue.
However, I still believe that the first patch is correct as any ports it
breaks has tests that depend on non-standard install(1) behavior caused
by the perl wrapper so including ${WRKDIR}/bin in PATH is just
concealing the breakage.
This directory is used for other things which may get called during
test, for example symlinks bin/python - /usr/local/bin/python-2.7 or
bin/cc - /usr/local/bin/egcc, so I think that the patch which removes
${WRKDIR}/bin from PATH during test is incorrect.
+ @rm -f ${WRKDIR}/bin/chown ${WRKDIR}/bin/chgrp
If you're going to do this, wouldn't it make sense to rm the install
wrapper as well?
There might be a problem though, I suspect it will break things with
SEPARATE_BUILD=flavored, so from that perspective it would be safer
to target it tightly in the port which is actually affected by it
even if it's not quite correct.
Re: [patch] Test target PATH
On Thu, Aug 27, 2015 at 11:50:19AM +0200, Juan Francisco Cantero Hurtado wrote:
Can you try this in your port?
TEST_ENV=PATH=${PATH}:YOURPATH or TEST_ENV=PATH=YOURPATH:${PATH}
I don't think you're following the problem. It's not that anything needs
to be added to PATH; it's that the first element needs to be removed.
The PATH that ports uses is set by PORTPATH to be
${WRKDIR}/bin:/usr/bin:/bin:/usr/sbin:/sbin:${DEPBASE}/bin:${LOCALBASE}/bin:${X11BASE}/bin
With make test, ${WRKDIR}/bin only has install in it, so chgrp runs
/bin/chgrp. When make fake is run, a symlink from ${WRKDIR}/bin/chgrp to
/bin/echo is added. This resuls in make fake test using the chgrp in
${WRKDIR}/bin and the test failing since that chgrp doesn't change the
group. I am proposing to remove ${WRKDIR}/bin from PATH for tests so
that all commands work as expected.
-Matthew Martin
On Thu, Aug 27, 2015 at 12:37:22AM -0500, Matthew Martin wrote:
The zsh port fails an additional test when invoked with make clean fake
test instead of make clean test. This is because in its tests it
touch zerolength
chgrp $EGID zerolength
and then tests that zerolength has a group of EGID. Since make fake puts
a symlink in ${WRKDIR}/bin from chgrp to /bin/echo and ${WRKDIR}/bin is
first in the environment's PATH, the chgrp does nothing, and test
C02cond fails. With the below patch it completes successfully.
This patch does slightly change behavior as PATH can no longer be set in
TEST_ENV (which no ports seem to do anyway) and install is no longer the
perl wrapper in ${WRKDIR}/bin; however, during tests we probably
shouldn't be messing with install either.
- Matthew Martin
Index: infrastructure/mk/bsd.port.mk
===
RCS file: /cvs/ports/infrastructure/mk/bsd.port.mk,v
retrieving revision 1.1298
diff -u -p -r1.1298 bsd.port.mk
--- infrastructure/mk/bsd.port.mk 19 Jul 2015 17:31:44 - 1.1298
+++ infrastructure/mk/bsd.port.mk 27 Aug 2015 03:53:55 -
@@ -809,7 +809,7 @@ FAKE_TARGET ?= ${INSTALL_TARGET}
TEST_TARGET ?= test
TEST_FLAGS ?=
-TEST_ENV ?=
+TEST_ENV +=
PATH=/usr/bin:/bin:/usr/sbin:/sbin:${DEPBASE}/bin:${LOCALBASE}/bin:${X11BASE}/bin
ALL_TEST_FLAGS = ${MAKE_FLAGS} ${TEST_FLAGS}
ALL_TEST_ENV = ${MAKE_ENV} ${TEST_ENV}
TEST_LOGFILE ?= ${WRKDIR}/test.log
--
Juan Francisco Cantero Hurtado http://juanfra.info
Re: [patch] Test target PATH
I see the problem now. You can remove the symlinks from ${WRKDIR}/bin with:
pre-test:
---rm -f ${WRKDIR}/bin/chgrp
The problem with your patch is that you're modifying the environment
for thousands of ports and we can't test manually every port to see if
your changes are breaking something.
On Thu, Aug 27, 2015 at 12:47:24PM -0500, Matthew Martin wrote:
On Thu, Aug 27, 2015 at 11:50:19AM +0200, Juan Francisco Cantero Hurtado
wrote:
Can you try this in your port?
TEST_ENV=PATH=${PATH}:YOURPATH or TEST_ENV=PATH=YOURPATH:${PATH}
I don't think you're following the problem. It's not that anything needs
to be added to PATH; it's that the first element needs to be removed.
The PATH that ports uses is set by PORTPATH to be
${WRKDIR}/bin:/usr/bin:/bin:/usr/sbin:/sbin:${DEPBASE}/bin:${LOCALBASE}/bin:${X11BASE}/bin
With make test, ${WRKDIR}/bin only has install in it, so chgrp runs
/bin/chgrp. When make fake is run, a symlink from ${WRKDIR}/bin/chgrp to
/bin/echo is added. This resuls in make fake test using the chgrp in
${WRKDIR}/bin and the test failing since that chgrp doesn't change the
group. I am proposing to remove ${WRKDIR}/bin from PATH for tests so
that all commands work as expected.
-Matthew Martin
On Thu, Aug 27, 2015 at 12:37:22AM -0500, Matthew Martin wrote:
The zsh port fails an additional test when invoked with make clean fake
test instead of make clean test. This is because in its tests it
touch zerolength
chgrp $EGID zerolength
and then tests that zerolength has a group of EGID. Since make fake puts
a symlink in ${WRKDIR}/bin from chgrp to /bin/echo and ${WRKDIR}/bin is
first in the environment's PATH, the chgrp does nothing, and test
C02cond fails. With the below patch it completes successfully.
This patch does slightly change behavior as PATH can no longer be set in
TEST_ENV (which no ports seem to do anyway) and install is no longer the
perl wrapper in ${WRKDIR}/bin; however, during tests we probably
shouldn't be messing with install either.
- Matthew Martin
Index: infrastructure/mk/bsd.port.mk
===
RCS file: /cvs/ports/infrastructure/mk/bsd.port.mk,v
retrieving revision 1.1298
diff -u -p -r1.1298 bsd.port.mk
--- infrastructure/mk/bsd.port.mk 19 Jul 2015 17:31:44 - 1.1298
+++ infrastructure/mk/bsd.port.mk 27 Aug 2015 03:53:55 -
@@ -809,7 +809,7 @@ FAKE_TARGET ?= ${INSTALL_TARGET}
TEST_TARGET ?= test
TEST_FLAGS ?=
-TEST_ENV ?=
+TEST_ENV +=
PATH=/usr/bin:/bin:/usr/sbin:/sbin:${DEPBASE}/bin:${LOCALBASE}/bin:${X11BASE}/bin
ALL_TEST_FLAGS = ${MAKE_FLAGS} ${TEST_FLAGS}
ALL_TEST_ENV = ${MAKE_ENV} ${TEST_ENV}
TEST_LOGFILE ?= ${WRKDIR}/test.log
--
Juan Francisco Cantero Hurtado http://juanfra.info
Re: [patch] Test target PATH
On Thu, Aug 27, 2015 at 11:04 PM, Matthew Martin phy1...@gmail.com wrote:
On Thu, Aug 27, 2015 at 11:47:19PM +0200, Juan Francisco Cantero Hurtado
wrote:
---rm -f ${WRKDIR}/bin/chgrp
While this would fix the problem, since the problem is not in zsh
itself, I don't think that the zsh port is the right place to fix this.
Since the problem is not in zsh itself, that means that zsh itself is
not the right place to fix this.
Since the bug only occurs in the port of zsh, that means that the zsh
port is the right place to fix this.
Or am I missing some important issue? (If so, could you explain what breaks?)
Thanks,
--
Raul
Re: [patch] Test target PATH
On Thu, Aug 27, 2015 at 11:47:19PM +0200, Juan Francisco Cantero Hurtado wrote:
I see the problem now. You can remove the symlinks from ${WRKDIR}/bin with:
pre-test:
---rm -f ${WRKDIR}/bin/chgrp
While this would fix the problem, since the problem is not in zsh
itself, I don't think that the zsh port is the right place to fix this.
The problem with your patch is that you're modifying the environment
for thousands of ports and we can't test manually every port to see if
your changes are breaking something.
However, overriding chown and chgrp during test is a bug. They clearly
don't need to be overridden during test since the symlinks only exist if
test is called after fake and don't exist for make clean test.
${WRKDIR}/bin is only in PATH during extract, configure, build, fake,
and test. Since fake creates the chown and chgrp symlinks, extract,
configure, build, and test must not depend on those symlinks as they
don't depend on fake. Would it be acceptable to clean up the symlinks
after fake completes or rm -f them in test in case test is run after
fake? Both patches below fix the issue.
However, I still believe that the first patch is correct as any ports it
breaks has tests that depend on non-standard install(1) behavior caused
by the perl wrapper so including ${WRKDIR}/bin in PATH is just
concealing the breakage.
- Matthew Martin
Index: infrastructure/mk/bsd.port.mk
===
RCS file: /cvs/ports/infrastructure/mk/bsd.port.mk,v
retrieving revision 1.1298
diff -u -p -r1.1298 bsd.port.mk
--- infrastructure/mk/bsd.port.mk 19 Jul 2015 17:31:44 - 1.1298
+++ infrastructure/mk/bsd.port.mk 28 Aug 2015 01:58:04 -
@@ -2886,6 +2886,8 @@ ${_FAKE_COOKIE}: ${_BUILD_COOKIE} ${_FAK
fi; \
done
+ @rm -f ${WRKDIR}/bin/chown ${WRKDIR}/bin/chgrp
+
@${_FAKESUDO} ${_MAKE_COOKIE} $@
print-plist:
Index: infrastructure/mk/bsd.port.mk
===
RCS file: /cvs/ports/infrastructure/mk/bsd.port.mk,v
retrieving revision 1.1298
diff -u -p -r1.1298 bsd.port.mk
--- infrastructure/mk/bsd.port.mk 19 Jul 2015 17:31:44 - 1.1298
+++ infrastructure/mk/bsd.port.mk 28 Aug 2015 02:06:13 -
@@ -2786,6 +2786,7 @@ ${_TEST_COOKIE}: ${_BUILD_COOKIE}
@exit 1
.endif
. endif
+ @rm -f ${WRKDIR}/bin/chown ${WRKDIR}/bin/chgrp
. if target(pre-test)
@${_MAKE} pre-test
. endif
Re: [patch] Test target PATH
Can you try this in your port?
TEST_ENV=PATH=${PATH}:YOURPATH or TEST_ENV=PATH=YOURPATH:${PATH}
On Thu, Aug 27, 2015 at 12:37:22AM -0500, Matthew Martin wrote:
The zsh port fails an additional test when invoked with make clean fake
test instead of make clean test. This is because in its tests it
touch zerolength
chgrp $EGID zerolength
and then tests that zerolength has a group of EGID. Since make fake puts
a symlink in ${WRKDIR}/bin from chgrp to /bin/echo and ${WRKDIR}/bin is
first in the environment's PATH, the chgrp does nothing, and test
C02cond fails. With the below patch it completes successfully.
This patch does slightly change behavior as PATH can no longer be set in
TEST_ENV (which no ports seem to do anyway) and install is no longer the
perl wrapper in ${WRKDIR}/bin; however, during tests we probably
shouldn't be messing with install either.
- Matthew Martin
Index: infrastructure/mk/bsd.port.mk
===
RCS file: /cvs/ports/infrastructure/mk/bsd.port.mk,v
retrieving revision 1.1298
diff -u -p -r1.1298 bsd.port.mk
--- infrastructure/mk/bsd.port.mk 19 Jul 2015 17:31:44 - 1.1298
+++ infrastructure/mk/bsd.port.mk 27 Aug 2015 03:53:55 -
@@ -809,7 +809,7 @@ FAKE_TARGET ?= ${INSTALL_TARGET}
TEST_TARGET ?= test
TEST_FLAGS ?=
-TEST_ENV ?=
+TEST_ENV +=
PATH=/usr/bin:/bin:/usr/sbin:/sbin:${DEPBASE}/bin:${LOCALBASE}/bin:${X11BASE}/bin
ALL_TEST_FLAGS = ${MAKE_FLAGS} ${TEST_FLAGS}
ALL_TEST_ENV = ${MAKE_ENV} ${TEST_ENV}
TEST_LOGFILE ?= ${WRKDIR}/test.log
--
Juan Francisco Cantero Hurtado http://juanfra.info
