Delete the leftover ${WRKSRC}/.metadir directory when 'make install' fails for any reason
ClosedPublic

Authored by mat on Jan 8 2018, 6:04 PM.

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
yuri created this revision.Jan 8 2018, 6:04 PM
yuri added a comment.Jan 8 2018, 6:06 PM

This problem happens when during development 'make install' fails due to, for example, a duplicate file conflict.

yuri retitled this revision from Deletes the leftover ${WRKSRC}/.metadir directory when 'make install' fails for any reason to Delete the leftover ${WRKSRC}/.metadir directory when 'make install' fails for any reason.Jan 8 2018, 10:33 PM
mat added a comment.Jan 9 2018, 1:08 PM

I don't think this is the right fix.

yuri added a comment.Jan 9 2018, 6:11 PM
In D13798#289393, @mat wrote:

I don't think this is the right fix.

  1. It accomplishes the cleanup, which is obviously missing in this place
  2. Tests confirm that it solves the original problem.

So, what is exactly not right about it?

These files do not need root permissions. I suggest we fix THAT.

yuri added a comment.Jan 24 2018, 10:01 PM

These files do not need root permissions. I suggest we fix THAT.

Once the make install command runs as root, it creates them with root credentials. It is possible to create them with credentials matching the work directory. But besides looking a bit stilted, this also has a downside of leaving these temporary files in case of failure.

tobik added a subscriber: tobik.Jan 24 2018, 11:01 PM

I've run into this bug many times before. Thank you for bringing this up Yuri.

In D13798#294776, @yuri wrote:

These files do not need root permissions. I suggest we fix THAT.

Once the make install command runs as root, it creates them with root credentials.

This begs the question if create-manifest really needs to be in _INSTALL_SUSEQ (as part of fake-pkg) or if it can simply be in _INSTALL_SEQ?

Something like

diff --git a/Mk/bsd.port.mk b/Mk/bsd.port.mk
index 3db728b11168..cd6f75c10fd8 100644
--- a/Mk/bsd.port.mk
+++ b/Mk/bsd.port.mk
@@ -4671,7 +4671,7 @@ flavors-package-names: .PHONY
 STAGE_ARGS=		-i ${STAGEDIR}
 
 .if !defined(NO_PKG_REGISTER)
-fake-pkg: create-manifest
+fake-pkg:
 .if defined(INSTALLS_DEPENDS)
 	@${ECHO_MSG} "===>   Registering installation for ${PKGNAME} as automatic"
 .else
@@ -5410,8 +5410,9 @@ _TEST_SEQ=		100:test-message 150:test-depends 300:pre-test 500:do-test \
 				${_OPTIONS_test} ${_USES_test}
 _INSTALL_DEP=	stage
 _INSTALL_SEQ=	100:install-message \
-				200:check-already-installed
-_INSTALL_SUSEQ=	300:fake-pkg 500:security-check
+				200:check-already-installed \
+				300:create-manifest
+_INSTALL_SUSEQ=	301:fake-pkg 500:security-check
 
 _PACKAGE_DEP=	stage
 _PACKAGE_SEQ=	100:package-message 300:pre-package 450:pre-package-script \

I've run into this bug many times before. Thank you for bringing this up Yuri.

In D13798#294776, @yuri wrote:

These files do not need root permissions. I suggest we fix THAT.

Once the make install command runs as root, it creates them with root credentials.

This begs the question if create-manifest really needs to be in _INSTALL_SUSEQ (as part of fake-pkg) or if it can simply be in _INSTALL_SEQ?

I don't have time to review at the moment but I think you're right that it doesn't need to be in SUSEQ.

mat added a comment.Jan 25 2018, 8:46 AM
In D13798#289472, @yuri wrote:
In D13798#289393, @mat wrote:

I don't think this is the right fix.

  1. It accomplishes the cleanup, which is obviously missing in this place
  2. Tests confirm that it solves the original problem.

    So, what is exactly not right about it?

Because you're fixing the symptoms, not the cause.

mat commandeered this revision.Jan 25 2018, 2:12 PM
mat added a reviewer: yuri.
mat retitled this revision from Delete the leftover ${WRKSRC}/.metadir directory when 'make install' fails for any reason to Don't run create-manifest as root.
mat updated this revision to Diff 38448.Jan 25 2018, 2:14 PM
mat retitled this revision from Don't run create-manifest as root to Delete the leftover ${WRKSRC}/.metadir directory when 'make install' fails for any reason.

Take on the patch from @tobik.

mat added a comment.Jan 25 2018, 2:15 PM

Tested on a couple of ports, I cannot see any harm it can do.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 1 2018, 8:31 AM
Closed by commit rP460578: Don't run create-manifest as root. (authored by mat, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.