[PATCH] bsd.port.mk - make relation between GH_TAGNAME & GH_COMMIT more apparent (prevent actual bug regression)
Hi tech@
I'm the maintainer of www/otter-browser and I got caught while packaging
otter-browser 0.9.04. Upstream asked us to point at a different commit
then the tagged revision so we did:
GH_TAGNAME = v0.9.04
# This is the actual tagged revision
# GH_COMMIT = 869d29d19719b3057e137a79d4a10025d2c920f6
# but we were asked by upstream to release from the following commit
# as it's considered an important fix affecting the majority of users
GH_COMMIT =23d7ee6f9cd636e750687a01975b177c1c9c2e53
This port was reviewed with an ok by two people and underwent further
changes later on.
I didn't notice that the port actually packaged GH_TAGNAME contents
instead of GH_COMMIT.
Current documentation for both tags are as follows:
GH_COMMIT SHA1 commit id to fetch. It is good practice to
always
specify the commit id, even if ${GH_TAGNAME} was
specified.
GH_TAGNAMEName of the tag to download. Setting ${GH_TAGNAME}
to
master is invalid and will throw an error.
${WRKDIST} is
auto-generated based on the ${GH_TAGNAME} if
specified,
otherwise ${GH_COMMIT} will be used to generate
${WRKDIST}.
I would like to suggest a small alteration to GH_COMMIT to point out
that
GH_TAGNAME takes precedence even if they point at different changeset.
The ports system doesn't warn about that situation and I almost got
caught
by it twice since upstream again asks us to package a couple of
revisions
ahead of the tagged version.
Regards,
Adam
Index: bsd.port.mk.5
===
RCS file: /cvs/src/share/man/man5/bsd.port.mk.5,v
retrieving revision 1.415
diff -u -p -r1.415 bsd.port.mk.5
--- bsd.port.mk.5 3 Apr 2015 10:19:22 - 1.415
+++ bsd.port.mk.5 4 Apr 2015 20:58:59 -
@@ -1703,6 +1703,7 @@ Account name of the GitHub user hosting
SHA1 commit id to fetch.
It is good practice to always specify
the commit id, even if ${GH_TAGNAME} was specified.
+${GH_TAGNAME} takes precedence even if ${GH_COMMIT} points at a
different changeset.
.It Ev GH_PROJECT
Name of the project on GitHub.
.It Ev GH_TAGNAME
Re: [PATCH] bsd.port.mk - make relation between GH_TAGNAME & GH_COMMIT more apparent (prevent actual bug regression)
On Sat, Apr 04, 2015 at 11:07:11PM +0200, Adam Wolk wrote:
> Hi tech@
>
> I'm the maintainer of www/otter-browser and I got caught while packaging
> otter-browser 0.9.04. Upstream asked us to point at a different commit
> then the tagged revision so we did:
>
> GH_TAGNAME = v0.9.04
> # This is the actual tagged revision
> # GH_COMMIT = 869d29d19719b3057e137a79d4a10025d2c920f6
> # but we were asked by upstream to release from the following commit
> # as it's considered an important fix affecting the majority of users
> GH_COMMIT =23d7ee6f9cd636e750687a01975b177c1c9c2e53
>
> This port was reviewed with an ok by two people and underwent further
> changes later on.
>
> I didn't notice that the port actually packaged GH_TAGNAME contents
> instead of GH_COMMIT.
GH_COMMIT is meaningless in terms of "package version", which expects a
correctly structured version, hence GH_TAGNAME being usually used in
combination with GH_PROJECT.
Look, you even set it yourself for otter-browser:
DISTNAME = ${GH_PROJECT}-${GH_TAGNAME:C/^v//}
(and PKGNAME is derived from DISTNAME)
Here, since you go forward in git history, you have the choice of
bumping REVISION, or using .MMDD suffixes, or using the special
'pre' / 'rc' / 'beta' keywords in the version, see packages-specs(7).
S i'm not sure the documentation is at fault here :)
Landry
Re: [PATCH] bsd.port.mk - make relation between GH_TAGNAME & GH_COMMIT more apparent (prevent actual bug regression)
On Sat, Apr 4, 2015, at 11:27 PM, Landry Breuil wrote:
> On Sat, Apr 04, 2015 at 11:07:11PM +0200, Adam Wolk wrote:
> > Hi tech@
> >
> > I'm the maintainer of www/otter-browser and I got caught while packaging
> > otter-browser 0.9.04. Upstream asked us to point at a different commit
> > then the tagged revision so we did:
> >
> > GH_TAGNAME = v0.9.04
> > # This is the actual tagged revision
> > # GH_COMMIT = 869d29d19719b3057e137a79d4a10025d2c920f6
> > # but we were asked by upstream to release from the following commit
> > # as it's considered an important fix affecting the majority of users
> > GH_COMMIT =23d7ee6f9cd636e750687a01975b177c1c9c2e53
> >
> > This port was reviewed with an ok by two people and underwent further
> > changes later on.
> >
> > I didn't notice that the port actually packaged GH_TAGNAME contents
> > instead of GH_COMMIT.
>
> GH_COMMIT is meaningless in terms of "package version", which expects a
> correctly structured version, hence GH_TAGNAME being usually used in
> combination with GH_PROJECT.
>
> Look, you even set it yourself for otter-browser:
>
> DISTNAME = ${GH_PROJECT}-${GH_TAGNAME:C/^v//}
>
> (and PKGNAME is derived from DISTNAME)
> Here, since you go forward in git history, you have the choice of
> bumping REVISION, or using .MMDD suffixes, or using the special
> 'pre' / 'rc' / 'beta' keywords in the version, see packages-specs(7).
>
> S i'm not sure the documentation is at fault here :)
>
> Landry
>
Yep, my fault. I should have tested this earlier.
ser/archive/v0.9.05/1ea5df13f908495df4ad9d634d997f6fd4c9.tar.gz
<
Trying 192.30.252.128...
Requesting
https://github.com/OtterBrowser/otter-browser/archive/v0.9.05/1ea5df13f908495df4a
d9d634d997f6fd4c9.tar.gz
Redirected to
https://codeload.github.com/OtterBrowser/otter-browser/tar.gz/v0.9.05
Trying 192.30.252.144...
Requesting
https://codeload.github.com/OtterBrowser/otter-browser/tar.gz/v0.9.05
100%
|**|
2248 KB00:05
2302624 bytes received in 5.65 seconds (398.11 KB/s)
So github redirects to the tag even though the ports system just shows a
download
===> Checking files for otter-browser-0.9.05
>> Fetch
>> https://github.com/OtterBrowser/otter-browser/archive/v0.9.05/1ea5df13f908495df4ad9d634d997f6fd4c9.tar.gz
I should pay more attention to what's going on during port building.
Feel free to ignore the
patch & thanks for feedback ;)
Regards,
Adam
Re: [PATCH] bsd.port.mk - make relation between GH_TAGNAME & GH_COMMIT more apparent (prevent actual bug regression)
On 2015-04-04, Landry Breuil wrote:
> On Sat, Apr 04, 2015 at 11:07:11PM +0200, Adam Wolk wrote:
>> Hi tech@
>>
>> I'm the maintainer of www/otter-browser and I got caught while packaging
>> otter-browser 0.9.04. Upstream asked us to point at a different commit
>> then the tagged revision so we did:
>>
>> GH_TAGNAME = v0.9.04
>> # This is the actual tagged revision
>> # GH_COMMIT = 869d29d19719b3057e137a79d4a10025d2c920f6
>> # but we were asked by upstream to release from the following commit
>> # as it's considered an important fix affecting the majority of users
>> GH_COMMIT =23d7ee6f9cd636e750687a01975b177c1c9c2e53
>>
>> This port was reviewed with an ok by two people and underwent further
>> changes later on.
>>
>> I didn't notice that the port actually packaged GH_TAGNAME contents
>> instead of GH_COMMIT.
>
> GH_COMMIT is meaningless in terms of "package version", which expects a
> correctly structured version, hence GH_TAGNAME being usually used in
> combination with GH_PROJECT.
Pretty much all ports with GH_TAGNAME are also setting GH_COMMIT, but
GH_COMMIT doesn't do anything there. I think we were hoping it would
fetch a specific commitid in case the tag was slided, but it doesn't
look like github supports that.
I think we should remove the existing ones and make it an error to
specify both GH_TAGNAME and GH_COMMIT. Thoughts? If people think this
is a good idea I'll do the ports mop-up.
Index: bsd.port.mk
===
RCS file: /cvs/ports/infrastructure/mk/bsd.port.mk,v
retrieving revision 1.1288
diff -u -p -r1.1288 bsd.port.mk
--- bsd.port.mk 4 Jan 2015 05:47:07 - 1.1288
+++ bsd.port.mk 5 Apr 2015 11:30:41 -
@@ -1168,6 +1168,9 @@ MASTER_SITE_OVERRIDE ?= No
.endif
.if !empty(GH_ACCOUNT) && !empty(GH_PROJECT)
+. if !empty(GH_COMMIT) && !empty(GH_TAGNAME)
+ERRORS += "Fatal: specifying both GH_TAGNAME and GH_COMMIT is invalid"
+. endif
. if ${GH_TAGNAME} == master
ERRORS += "Fatal: using master as GH_TAGNAME is invalid"
. endif
Re: [PATCH] bsd.port.mk - make relation between GH_TAGNAME & GH_COMMIT more apparent (prevent actual bug regression)
On Sun, Apr 5, 2015, at 01:31 PM, Stuart Henderson wrote:
> On 2015-04-04, Landry Breuil wrote:
> > On Sat, Apr 04, 2015 at 11:07:11PM +0200, Adam Wolk wrote:
> >> Hi tech@
> >>
> >> I'm the maintainer of www/otter-browser and I got caught while packaging
> >> otter-browser 0.9.04. Upstream asked us to point at a different commit
> >> then the tagged revision so we did:
> >>
> >> GH_TAGNAME = v0.9.04
> >> # This is the actual tagged revision
> >> # GH_COMMIT = 869d29d19719b3057e137a79d4a10025d2c920f6
> >> # but we were asked by upstream to release from the following commit
> >> # as it's considered an important fix affecting the majority of users
> >> GH_COMMIT =23d7ee6f9cd636e750687a01975b177c1c9c2e53
> >>
> >> This port was reviewed with an ok by two people and underwent further
> >> changes later on.
> >>
> >> I didn't notice that the port actually packaged GH_TAGNAME contents
> >> instead of GH_COMMIT.
> >
> > GH_COMMIT is meaningless in terms of "package version", which expects a
> > correctly structured version, hence GH_TAGNAME being usually used in
> > combination with GH_PROJECT.
>
> Pretty much all ports with GH_TAGNAME are also setting GH_COMMIT, but
> GH_COMMIT doesn't do anything there. I think we were hoping it would
> fetch a specific commitid in case the tag was slided, but it doesn't
> look like github supports that.
>
I can confirm this. That's how I got burned with otter. Ports 'tell you'
that they
are downloading from that 'tag' pluus the GH_COMMIT you set making you
think it's actually downloading the proper changeset.
In reality, github does redirects behind the scene and points at the
tagged
revision.
> I think we should remove the existing ones and make it an error to
> specify both GH_TAGNAME and GH_COMMIT. Thoughts? If people think this
> is a good idea I'll do the ports mop-up.
>
> Index: bsd.port.mk
> ===
> RCS file: /cvs/ports/infrastructure/mk/bsd.port.mk,v
> retrieving revision 1.1288
> diff -u -p -r1.1288 bsd.port.mk
> --- bsd.port.mk 4 Jan 2015 05:47:07 - 1.1288
> +++ bsd.port.mk 5 Apr 2015 11:30:41 -
> @@ -1168,6 +1168,9 @@ MASTER_SITE_OVERRIDE ?= No
> .endif
>
> .if !empty(GH_ACCOUNT) && !empty(GH_PROJECT)
> +. if !empty(GH_COMMIT) && !empty(GH_TAGNAME)
> +ERRORS += "Fatal: specifying both GH_TAGNAME and GH_COMMIT is invalid"
> +. endif
> . if ${GH_TAGNAME} == master
> ERRORS += "Fatal: using master as GH_TAGNAME is invalid"
> . endif
>
>
I think it is a good idea. I would also suggest changing the docs to no
longer
suggest that it is good practice to set both.
Index: bsd.port.mk.5
===
RCS file: /cvs/src/share/man/man5/bsd.port.mk.5,v
retrieving revision 1.415
diff -u -p -r1.415 bsd.port.mk.5
--- bsd.port.mk.5 3 Apr 2015 10:19:22 - 1.415
+++ bsd.port.mk.5 5 Apr 2015 12:26:41 -
@@ -1701,8 +1701,7 @@ Yields a suitable default for
Account name of the GitHub user hosting the project.
.It Ev GH_COMMIT
SHA1 commit id to fetch.
-It is good practice to always specify
-the commit id, even if ${GH_TAGNAME} was specified.
+It is an error to specify ${GH_COMMIT} when ${GH_TAGNAME} is specified.
.It Ev GH_PROJECT
Name of the project on GitHub.
.It Ev GH_TAGNAME
Re: [PATCH] bsd.port.mk - make relation between GH_TAGNAME & GH_COMMIT more apparent (prevent actual bug regression)
Stuart Henderson said: > I think we should remove the existing ones and make it an error to > specify both GH_TAGNAME and GH_COMMIT. Thoughts? If people think this > is a good idea I'll do the ports mop-up. I'd rather see a warning saying that GH_COMMIT is ignored and should be removed. I see no reason to bump all ports that currently specify both. -- Dmitrij D. Czarkoff
Re: [PATCH] bsd.port.mk - make relation between GH_TAGNAME & GH_COMMIT more apparent (prevent actual bug regression)
On 2015/04/06 11:59, Dmitrij D. Czarkoff wrote: > Stuart Henderson said: > > I think we should remove the existing ones and make it an error to > > specify both GH_TAGNAME and GH_COMMIT. Thoughts? If people think this > > is a good idea I'll do the ports mop-up. > > I'd rather see a warning saying that GH_COMMIT is ignored and should be > removed. I see no reason to bump all ports that currently specify both. This way it's crystal clear; people frequently copy an existing port as a template (rather than using Makefile.template) so it's useful to remove examples of the "wrong way to do it".
