On 13/07/2022 02:52, Stuart Henderson wrote:
> On 2022/07/12 08:41, Aaron Bieber wrote:
>> On Mon, 11 Jul 2022 at 11:17:55 +0000, Klemens Nanni wrote:
>>> GH_* ports obviously set MASTER_SITES to MASTER_SITES_GITHUB which
>>> defaults github.com/${account}/${project}/archive/${commit-or-tag}.
>>>
>>> If additional patches need to be fetched, e.g. pending PRs to fix the
>>> local port, additional MASTER_SITES0-9 must be defined which always
>>> duplicate the GH_* values:
>>>
>>> MASTER_SITES0 = https://github.com/account/project/pull/
>>> PATCHFILES += many_fixes-{}number.patch:0
>>> or
>>> MASTER_SITES0 = https://github.com/account/project/commit/
>>> PATCHFILES += one_fix-{}id.patch:0
>>> or
>>> MASTER_SITES0 = https://github.com/account/project/
>>> PATCHFILES += many_fixes-{pull/}number.patch:0
>>> PATCHFILES += one_fix-{commit/}id.patch:0
>>>
>>> Not super important but a bit annoying, the automatically generated
>>> MASTER_SITES_GITHUB can't really be reused (without dirty make fiddling)
>>> so MASTER_SITES0 is always a duplicate and PATCHFILES need :0 to work.
>>>
>>>
>>> But all we need to make this reusable is move the "archive/" part from
>>> MASTER_SITES_GITHUB to GH_DISTFILE, i.e. internally do
>>>
>>> MASTER_SITES = https://github.com/account/project/
>>> DISTNAME = account-project-{archive/}commit_or_tag.tar.gz
>>> instead of
>>> MASTER_SITES = https://github.com/account/project/archive/
>>> DISTNAME = account-project-{}commit_or_tag.tar.gz
>
> If that is changing I think it might be better to go for
>
> MASTER_SITES = https://github.com/
> DISTNAME = account-project-{account/project/archive/}commit_or_tag.tar.gz
>
> Then we don't need multiple MASTER_SITES for a second github distfile either
>
>>> since this way the aforementioned patch fetching can be simplified to
>>>
>>> PATCHFILES += many_fixes-{pull/}number.patch
>>> or
>>> PATCHFILES += one_fix-{commit/}id.patch
>>>
>>> without additional MASTER_SITES0-9.
>>>
>>> It's a rather unimportant detail but juggling with WIP ports, own PRs,
>>> fixes, etc. gets a little easier with this since the Makefile stays a
>>> bit smaller.
>>>
>>> For testing, I have fetched several ports, both using GH_COMMIT and
>>> GH_TAGNAME, without any failures or distinfo changes.
>>>
>>> As for the logic in bsd.port.mk itself, this also lifts the quirky
>>> add-trailing-slash-to-MASTER_SITES_GITHUB-only-if-GH_TAGNAME-is-set
>>> since it now ends with a fixed string and the additional URL paths only
>>> come in where GH_TAGNAME or whatever follows is definitely set.
>>>
>>> Feedback? Objection? OK?
>>>
>>
>> I like it! I can't speak to the make voodoo but the functionality is OK
>> abieber@ :D
>
> I worry that these upstream patchfiles might become inconsistent fairly
> easily (even more than git-archive tarballs). A post-commit tweak to the
> commit log message or an edit to the PR will break fetches because the
> file will change.
That's only true if /pull/<number>.patch is used and certainly a valid
point, but that's already a problem regardless of this diff.
<commit>.patch URLs remain the same, we'd just keep fetching the old
stuff if someone updated a PR or force pushed to a repo.
> Another problem is that it can be used to misrepresent patches as being
> real repo commits when they are spoofed. e.g. this was never committed to
> the real github/dmca repo, yet the URL looks official:
> https://github.com/github/dmca/commit/48c5663c5f7dd9ecc4720f7c1522627665197939.patch
While this is a fundemantally GitHub specific issue, we can try our best
to avoid shady URLs by using author/project/pull/<number>/<commit>
instead of author/project/<commit>.
I myself usually annotate fetched patches with
# pending "do this"
# https://link.to/PR/or/so
Does the proposed change make it easier to misuse GitHub URLs?
> If you view the non-patch version it does look a bit more suspicious with
> the header on the page, but there's no cue like that in a port with such a line
> https://github.com/github/dmca/commit/48c5663c5f7dd9ecc4720f7c1522627665197939
>
> (Method at https://gist.github.com/lrvick/02088ee5466ca51116bdaf1e709ddd7c)
>
>
>
>> patches upstream.. for example:
>> https://patch-diff.githubusercontent.com/raw/tailscale/tailscale/pull/4838.patch )
>>
>> I wonder if it would make sense to have an "alias" for a
>> MASTER_SITES entry that could be referenced in a more human
>> frindly/generic way. Maybe something like:
>>
>> PATCHFILES += github:account/project/many_fixes-{pull/}number.patch
>> PATCHFILES += gitlab:account/project/one_fix-{commit/}id.patch
>
> There's a lot of common handling between DISTFILES, PATCHFILES, SUPDISTFILES.
> Considering that some ports have thousands of distfiles I'd worry that more
> complex processing of this will really slow down make(1), it's already pretty
> bad in some cases.
Yes, we've had DISTFILES tweaks in go.port.mk with noticable performance
impacts already, so this should be looked after.
No comments:
Post a Comment