Bug 269584 - sysutils/debootstrap: can't chmod debootstrap
Summary: sysutils/debootstrap: can't chmod debootstrap
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Neel Chauhan
URL:
Keywords:
Depends on:
Blocks: 269631
  Show dependency treegraph
 
Reported: 2023-02-16 14:03 UTC by Gary
Modified: 2023-03-09 20:56 UTC (History)
5 users (show)

See Also:
bugzilla: maintainer-feedback? (nc)


Attachments
poudriere logfile (16.83 KB, text/plain)
2023-02-16 14:03 UTC, Gary
no flags Details
[patch] fix broken Makefile patch (1.07 KB, patch)
2023-02-17 15:44 UTC, John Hein
no flags Details | Diff
[patch] fix broken Makefile patch (v2) (1.07 KB, patch)
2023-02-24 01:55 UTC, John Hein
no flags Details | Diff
[patch] fix more patching breakage fallout (v3) (1.58 KB, patch)
2023-02-24 16:29 UTC, John Hein
zirias: maintainer-approval-
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gary 2023-02-16 14:03:25 UTC
Created attachment 240180 [details]
poudriere logfile

See attached log
Comment 1 John Hein 2023-02-17 15:44:36 UTC
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 2 John Hein 2023-02-17 15:47:24 UTC
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.
Comment 3 John Hein 2023-02-24 01:05:43 UTC
See ports c77cf9d7fac39f1cae64ba1db5c2d4209e4520bc (commits part of the patch - attachment 240215 [details]).
Comment 4 John Hein 2023-02-24 01:55:14 UTC
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.
Comment 5 Felix Palmen freebsd_committer freebsd_triage 2023-02-24 07:08:42 UTC
(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.
Comment 6 Felix Palmen freebsd_committer freebsd_triage 2023-02-24 08:39:10 UTC
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
Comment 7 Felix Palmen freebsd_committer freebsd_triage 2023-02-24 10:13:30 UTC
(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.
Comment 8 John Hein 2023-02-24 16:00:54 UTC
(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.
Comment 9 John Hein 2023-02-24 16:08:02 UTC
(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.
Comment 10 John Hein 2023-02-24 16:08:48 UTC
(In reply to John Hein from comment #9)
Make that ports 90bac03a2eef13458258f034ea421e572114c12b
Comment 11 Felix Palmen freebsd_committer freebsd_triage 2023-02-24 16:14:24 UTC
(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 ;)
Comment 12 John Hein 2023-02-24 16:24:48 UTC
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.
Comment 13 John Hein 2023-02-24 16:26:56 UTC
(In reply to John Hein from comment #12)
comment 12 submitted after your comment 11 obviously.
Comment 14 John Hein 2023-02-24 16:29:44 UTC
Created attachment 240382 [details]
[patch] fix more patching breakage fallout (v3)

This addresses the three breakages from comment 12.
Comment 15 John Hein 2023-02-24 16:32:26 UTC
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.
Comment 16 Felix Palmen freebsd_committer freebsd_triage 2023-02-24 16:33:52 UTC
(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.
Comment 17 Felix Palmen freebsd_committer freebsd_triage 2023-02-24 17:03:33 UTC
(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
Comment 18 John Hein 2023-02-24 17:04:34 UTC
(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.
Comment 19 Felix Palmen freebsd_committer freebsd_triage 2023-02-24 17:06:50 UTC
(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 ;)
Comment 20 John Hein 2023-02-24 17:11:27 UTC
(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).
Comment 21 Felix Palmen freebsd_committer freebsd_triage 2023-02-24 17:17:02 UTC
(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...
Comment 22 John Hein 2023-02-24 17:31:52 UTC
(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.
Comment 23 John Hein 2023-02-24 17:45:30 UTC
(In reply to John Hein from comment #22)
Added a comment in the review about pcregrep (or pcre2grep) over gnugrep (see bug 255525)
Comment 24 Felix Palmen freebsd_committer freebsd_triage 2023-02-24 17:52:56 UTC
(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.
Comment 25 John Hein 2023-02-24 18:04:19 UTC
(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 26 Felix Palmen freebsd_committer freebsd_triage 2023-02-26 21:17:59 UTC
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 :-)
Comment 27 commit-hook freebsd_committer freebsd_triage 2023-03-07 17:10:51 UTC
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(-)
Comment 28 Jesús Daniel Colmenares Oviedo 2023-03-09 00:29:49 UTC
(In reply to commit-hook from comment #27)

Shouldn't this bug be closed (CLOSED/FIXED)?
Comment 29 Felix Palmen freebsd_committer freebsd_triage 2023-03-09 10:44:29 UTC
(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 ;)
Comment 30 John Hein 2023-03-09 14:28:55 UTC
(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).
Comment 31 Gleb Popov freebsd_committer freebsd_triage 2023-03-09 15:11:06 UTC
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.
Comment 32 John Hein 2023-03-09 20:00:18 UTC
(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.
Comment 33 Felix Palmen freebsd_committer freebsd_triage 2023-03-09 20:56:45 UTC
(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.