Created attachment 240180 [details] poudriere logfile See attached log
Created attachment 240215 [details] [patch] fix broken Makefile patch The patch-Makefile change that was recently committed for 1.0.128_1 is broken in two ways: (1) a chmod invocation for sbin/debootstrap was placed before the file is created (2) /usr/local is hard-coded which is a regression from the previous versions of patch-Makefile The patch attached here fixes both (1) and (2). Note that in prior versions of patch-Makefile, the chmod was just removed. In that case the permissions for sbin/debootstrap were explicitly set to 0755 in the pkg-plist. For 1.0.28, nc@ removed that pkg-plist which triggered problems and a bug report (see bug 268205, comment 8). For the patch attached here, I left the chmod in patch-Makefile, so restoring the pkg-plist method of setting execute permissions is not necessary. There's not much difference between either method (chmod in Makefile or 0755 setting in pkg-plist). Leaving the chmod in patch-Makefile makes pkg-plist slightly cleaner. And it could allow direct execution in the staging directory (but running debootstrap in stage/ would require setting the DEBOOTSTRAP_DIR environment variable and maybe other settings changes as well, so that's not much of a benefit).
Comment on attachment 240215 [details] [patch] fix broken Makefile patch When regenerating patch-Makefile in the future, be careful when using 'make makepatch'. Any patches generated by makepatch will be AFTER the %%XXX%% strings have been expanded. So remember to restore any %%XXX%% patterns (e.g., %%PREFIX%%, %%DATADIR%%) to their unexpanded state before committing results from makepatch.
See ports c77cf9d7fac39f1cae64ba1db5c2d4209e4520bc (commits part of the patch - attachment 240215 [details]).
Created attachment 240358 [details] [patch] fix broken Makefile patch (v2) Refresh patch after ports c77cf9d7fac39f1cae64ba1db5c2d4209e4520bc. This now just fixes the %% substitutions that were incorrectly removed recently. The chmod ordering fix in the previous patch is now correct after the above commit.
(In reply to John Hein from comment #4) > This now just fixes the %% substitutions that were incorrectly removed recently. Unfortunately, I've seen this PR only *after* committing my fix right now. I didn't notice substitutions were done on the Makefile as well. IMHO, they are not needed, make variables are passed to other instances, and as the Makefile must be patched anyways, just use the variables there. Then, 'Makefile' could be removed from the substitutions in post-patch, avoiding this source of errors in the future.
Looks like I accidentally broke it again, trying to "simplify" a sane derived pkg version too much. Here's a review that should hopefully fix all issues, please have a look: https://reviews.freebsd.org/D38754
(In reply to John Hein from comment #2) > When regenerating patch-Makefile in the future, be careful when using 'make > makepatch'. Any patches generated by makepatch will be AFTER the %%XXX%% > strings have been expanded. So remember to restore any %%XXX%% patterns > (e.g., %%PREFIX%%, %%DATADIR%%) to their unexpanded state before committing > results from makepatch. Just a little hint, there's a simpler way to handle this. Apply patches with make BATCH=1 extract do-patch so post-patch is not executed. Then you can edit the files und use 'make makepatch' without issues.
(In reply to Felix Palmen from comment #7) > Just a little hint, there's a simpler way to handle this. Apply patches with > > make BATCH=1 extract do-patch Depending on the implementation of do-patch, yes, that is often useful. As one example exceptional case, devel/arduino-builder would not work - it expands %% patterns in do-patch itself. It's always wise to review and understand all your diffs before you commit them (some level diligence is important - learned from my own failures). Regarding your updated fix to remove %% subs and replace with passing in of variables, I agree with that approach here. It looks good to me (just viewed the diffs - not yet tested). Thanks for taking this.
(In reply to Felix Palmen from comment #7) I can't close this myself. Gary, if you are happy with the current state after ports 2bf4a73e61cd322efa426f55101afa25bd2481d3, please close this bug.
(In reply to John Hein from comment #9) Make that ports 90bac03a2eef13458258f034ea421e572114c12b
(In reply to John Hein from comment #9) > ports 2bf4a73e61cd322efa426f55101afa25bd2481d3 This still leaves one instance of hardcoded /usr/local (in the debootstrap script itself). Although not the core issue of this PR, I'd opt to leave it open until this is fixed as well, just awaiting Neel's comments ;)
Felix, there is another couple cases of hard-coded /usr/local I didn't notice: % grep usr/local files/* files/patch-debootstrap:+#!/usr/local/bin/bash files/patch-debootstrap:+ DEBOOTSTRAP_DIR=/usr/local/share/debootstrap There's not good way around using %%PREFIX%% for the DEBOOTSTRAP_DIR that I can see. Also the REINPLACE_CMD in post-patch for gpgv2 creates an erroneous call to gpgv22 in the final debootstrap. All three of these problems appear to be another casualty of not checking the results of 'makepatch' - broken with the commit to patch-debootstrap in f78f4879c494526e1bef0653c343753627b62cce.
(In reply to John Hein from comment #12) comment 12 submitted after your comment 11 obviously.
Created attachment 240382 [details] [patch] fix more patching breakage fallout (v3) This addresses the three breakages from comment 12.
Comment on attachment 240382 [details] [patch] fix more patching breakage fallout (v3) Adding Felix for patch review. Neel, if you want to weigh in here or commit the fixes, please do so.
(In reply to John Hein from comment #12) > % grep usr/local files/* > files/patch-debootstrap:+#!/usr/local/bin/bash Thanks, updated my review to fix this one as well. > files/patch-debootstrap:+DEBOOTSTRAP_DIR=/usr/local/share/debootstrap This however is not from the current version of my review ... > Also the REINPLACE_CMD in post-patch for gpgv2 creates an erroneous call to > gpgv22 in the final debootstrap. ... and this neither, this sub is removed because it's a static change that should be done with a patch file.
(In reply to John Hein from comment #15) > Adding Felix for patch review. I now understand you just worked on the current state in main. Indeed, this patches gpgv->gpgv2 twice, another goof that calls for yet another MFH. Pointy-hat to me again (I shouldn't have touched this, somehow it hates me ....) IMHO maintainer-approval is for the maintainer, so, I won't touch it, but I think: * no need for /usr/bin/env, we KNOW where bash will be installed below LOCALBASE * %%DATADIR%% should be used instead of %%PREFIX%%/share/debootstrap * static changes should really be done with patch-files, not REINPLACE_CMD Please refer here for my *updated* fix to finally get this port correct again: https://reviews.freebsd.org/D38754
(In reply to Felix Palmen from comment #16) The extra REINPLACE_CMD invocations trigger warnings about unnecessary subs in DEVELOPER=yes mode. I now have the following to make remove the warnings and gain some efficiency: post-patch: # Workaround for bin/255525 @${REINPLACE_CMD} 's,grep,${LOCALBASE}/bin/grep,g' ${WRKSRC}/functions @${REINPLACE_CMD} -e 's,%%PREFIX%%,${PREFIX},g' \ -e 's,%%DATADIR%%,${DATADIR},g' \ -e 's,%%LOCALBASE%%,${LOCALBASE},g' \ -e 's,gpgv ,gpgv2 ,g' \ ${WRKSRC}/debootstrap \ ${WRKSRC}/functions @${FIND} ${WRKSRC}/scripts -type f | xargs grep -l /usr/share/keyrings | \ ${XARGS} ${REINPLACE_CMD} -e 's,/usr/share/keyrings,${LOCALBASE}/share/keyrings,g' And with the static patch for gpgv2, the "-e 's,gpgv ,gpgv2 ,g'" can be removed, too. Passing ${WRKSRC}/Makefile to REINPLACE_CMD is no longer necessary after your commit to remove %% patterns from the Makefile patch.
(In reply to John Hein from comment #18) > The extra REINPLACE_CMD invocations trigger warnings about unnecessary subs > in DEVELOPER=yes mode. They always did, and that's kind of commonplace in many ports. I personally see no reason to "fix" this at the cost of added complexity, after all, it's broken. But then, up to Neel ;)
(In reply to Felix Palmen from comment #19) I agree that adding complexity is not worth fixing the empty reinplace warnings. But I think may changes in comment 18 are simpler (except for the extra grep -l - that's a bit of apple polishing that need not be committed).
(In reply to John Hein from comment #20) Please refer to https://reviews.freebsd.org/D38754 Do you see any issues with that? It removes even more of the post-patch stuff. Btw, I meant "it's NOT broken" of course. If you really want to "polish" there, make sure you use the variables (like ${XARGS}) instead of plain commands. I personally think it's not worth the effort just to "fix" some warning output...
(In reply to John Hein from comment #20) (In reply to Felix Pelman from comment #21) Here are even better simplifications (that apply to the review plus use of #!/usr/bin/env bash). post-patch: @${REINPLACE_CMD} 's,%%DATADIR%%,${DATADIR},g' ${WRKSRC}/debootstrap @${FIND} ${WRKSRC}/scripts -type f | xargs grep -l /usr/share/keyrings | \ ${XARGS} ${REINPLACE_CMD} -e 's,/usr/share/keyrings,${LOCALBASE}/share/keyrings,g' # Workaround for bin/255525 @${REINPLACE_CMD} 's,grep,${LOCALBASE}/bin/grep,g' ${WRKSRC}/functions The "xargs grep -l /usr/share/keyrings |" can be removed if we can tolerate the 'empty replacement' warning for the scripts files (with DEVELOPER=yes). In your review, I would use #!/usr/bin/env bash. One less sub for %%LOCALBASE%%, and I prefer using /usr/bin/env in shebangs generally. It gives one flexibility to set PATH to use a different utility. Some people like hard-coded paths better. I don't think that hard-coding adds to security or robustness much, if at all (I understand the arguments about path injection attacks, but generally don't find errors / exploitations of that in real life for well configured environments). Otherwise, I think your patches in the review look good.
(In reply to John Hein from comment #22) Added a comment in the review about pcregrep (or pcre2grep) over gnugrep (see bug 255525)
(In reply to John Hein from comment #22) Ok let's cut it short please. Your suggestion for post-patch is *almost* exactly the same as mine, short of the extra grep, plus my version also replaces LOCALBASE for the shebang... Talking about which, thanks for your suggestion, but in my personal opinion, /usr/bin/env should be avoided if you can know the real path -- for reasons you mentioned yourself ;) So, I'm now just awaiting Neel's assessment of the review.
(In reply to Felix Palmen from comment #24) In my usage, I prefer the ability to alter my path to point to a different version of a tool (e.g., bash) and just have things work without having to edit installed scripts. Much easier when testing alternate versions of tools (which I often need/want to do, likely different than the more common user I suppose). I understand the other point of view. See you back here after maintainer timeout ;)
Comment on attachment 240382 [details] [patch] fix more patching breakage fallout (v3) I'm declining this now as I'm getting reminders by bugzilla. I think my review already covers all remaining issues that *must* be addressed. Please feel free to add *additional* things, but please request approval from the actual maintainer of this port, thanks :-)
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=4e0f740ddca6bd6d4cf7017bb3132ee759f6db39 commit 4e0f740ddca6bd6d4cf7017bb3132ee759f6db39 Author: Felix Palmen <zirias@FreeBSD.org> AuthorDate: 2023-02-24 08:32:30 +0000 Commit: Felix Palmen <zirias@FreeBSD.org> CommitDate: 2023-03-07 17:09:28 +0000 sysutils/debootstrap: Fix several issues * Don't use substitutions on Makefile, pass variables instead * Eliminate "static" substitutions, use a patch-file instead * Fix hardcoded /usr/local in debootstrap script PR: 269584 Approved by: nc (maintainer), tcberner (mentor) Differential Revision: https://reviews.freebsd.org/D38754 sysutils/debootstrap/Makefile | 24 +++++++++--------------- sysutils/debootstrap/files/patch-Makefile | 2 +- sysutils/debootstrap/files/patch-debootstrap | 4 ++-- sysutils/debootstrap/files/patch-functions (new) | 20 ++++++++++++++++++++ 4 files changed, 32 insertions(+), 18 deletions(-)
(In reply to commit-hook from comment #27) Shouldn't this bug be closed (CLOSED/FIXED)?
(In reply to Jesús Daniel Colmenares Oviedo from comment #28) It's not assigned to me, but I'm pretty sure this latest commit should finally fix everything that actually needed fixing. So, I'll close it, if anyone disagrees there's always the option to add further comments ;)
(In reply to Felix Palmen from comment #29) (In reply to Jesús Daniel Colmenares Oviedo from comment #28) Maybe Jesús is wondering whether it should have been automatically closed/fixed due to the reference to the PR number in the commit message. I wonder why that didn't happen myself (or better: where to look in docs or source code to find out why).
We don't have such functionality in Bugzilla. The "PR:" tag merely adds a comment with information about the commit into the respective Bugzilla PR. It is up to a human to close it.
(In reply to Gleb Popov from comment #31) Ok. Thanks. It's a credit to committers that most of the time that does happen. Maybe it happens less than I perceive. I guess there could be a commit hook that at least spews a reminder to a committer to consider disposition of a PR if one is mentioned in the commit message.
(In reply to John Hein from comment #32) Ok I'll try to sort that out: First, this is a bug-tracking tool. It's not too uncommon that a commit is *related* to a bug, but doesn't (completely) solve it. Therefore, automatically closing the PR is not a good option. Also note how this is different from a code-review tool where the "issue" is some code to be reviewed and committed: Once it's committed, the review is obviously closed (therefore, this happens automatically on phabricator). Then, I don't think some extra notification would do any good. You normally won't reference a PR in a commit you're not subscribed to, so you will already receive the mail about your commit. To get it straight: The only reason I didn't close this right away was that I was not the assignee and I didn't want to overrule Neel. In hindsight, it probably wasn't wise to wait here, as you can always comment if you think the PR isn't really solved and have it reopened. Sorry for that.