| Message ID | 20161128103231.21626-1-damjan.marion@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Hi Damjan, On Mon, Nov 28, 2016 at 11:32:31AM +0100, Damjan Marion wrote: > tvheadedend tracks version information based on git version > numbers. As tvheadend is downloaded as tarball form github > it was reporting version 0.0.0~unknown. Having the version reported as v4.1-2345-g04ff649 is not all that helpful either, since commit 04ff649 is quite far from v4.1 in terms of both commits count and time. Broken version report should be fixed upstream, IMO. baruch > Signed-off-by: Damjan Marion <damjan.marion@gmail.com> > --- > package/tvheadend/tvheadend.hash | 2 +- > package/tvheadend/tvheadend.mk | 3 ++- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/package/tvheadend/tvheadend.hash b/package/tvheadend/tvheadend.hash > index ca10dc3..53cbccc 100644 > --- a/package/tvheadend/tvheadend.hash > +++ b/package/tvheadend/tvheadend.hash > @@ -1,2 +1,2 @@ > # Locally computed > -sha256 71dd3d8efa32d592342ff6955cc2cd317f07e74d2d546977eb9bc45136dd5de6 tvheadend-50a370707aedf5c127e92bb517c378aa8ac04657.tar.gz > +sha256 9af13b3e0381fa1f229eaf06acd8ad0d92013c2004a8005f1c66e56cd6d3a4d1 tvheadend-v4.1-2345-g04ff649.tar.gz > diff --git a/package/tvheadend/tvheadend.mk b/package/tvheadend/tvheadend.mk > index a3afec8..ad5d72d 100644 > --- a/package/tvheadend/tvheadend.mk > +++ b/package/tvheadend/tvheadend.mk > @@ -4,7 +4,7 @@ > # > ################################################################################ > > -TVHEADEND_VERSION = 50a370707aedf5c127e92bb517c378aa8ac04657 > +TVHEADEND_VERSION = v4.1-2345-g04ff649 > TVHEADEND_SITE = $(call github,tvheadend,tvheadend,$(TVHEADEND_VERSION)) > TVHEADEND_LICENSE = GPLv3+ > TVHEADEND_LICENSE_FILES = LICENSE.md > @@ -88,6 +88,7 @@ define TVHEADEND_CONFIGURE_CMDS > endef > > define TVHEADEND_BUILD_CMDS > + @echo $(TVHEADEND_VERSION) > $(@D)/rpm/version > $(TARGET_MAKE_ENV) $(MAKE) -C $(@D) > endef
> On 28 Nov 2016, at 11:44, Baruch Siach <baruch@tkos.co.il> wrote: > > Hi Damjan, > > On Mon, Nov 28, 2016 at 11:32:31AM +0100, Damjan Marion wrote: >> tvheadedend tracks version information based on git version >> numbers. As tvheadend is downloaded as tarball form github >> it was reporting version 0.0.0~unknown. > > Having the version reported as v4.1-2345-g04ff649 is not all that helpful > either, since commit 04ff649 is quite far from v4.1 in terms of both commits > count and time. Broken version report should be fixed upstream, IMO. Well, at least you know that it is 2345th commit after release 4.1 and that its commit id is 04ff649. This is much more information than “0.0.0~unknown” which buildroot compiled binary was reporting so far. Damjan
Hello, On Mon, 28 Nov 2016 11:32:31 +0100, Damjan Marion wrote: > tvheadedend tracks version information based on git version > numbers. As tvheadend is downloaded as tarball form github > it was reporting version 0.0.0~unknown. > > Signed-off-by: Damjan Marion <damjan.marion@gmail.com> Could you split the bump and the version fix reporting in two separate patches? > -TVHEADEND_VERSION = 50a370707aedf5c127e92bb517c378aa8ac04657 > +TVHEADEND_VERSION = v4.1-2345-g04ff649 Please keep this as a full hash, like it used to be. It should still work fine with the version reporting, if you write this version to $(@D)/rpm/version, like you're doing below. Thanks! Thomas
> On 28 Nov 2016, at 22:03, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > > Hello, > > On Mon, 28 Nov 2016 11:32:31 +0100, Damjan Marion wrote: >> tvheadedend tracks version information based on git version >> numbers. As tvheadend is downloaded as tarball form github >> it was reporting version 0.0.0~unknown. >> >> Signed-off-by: Damjan Marion <damjan.marion@gmail.com> > > Could you split the bump and the version fix reporting in two separate > patches? ack > >> -TVHEADEND_VERSION = 50a370707aedf5c127e92bb517c378aa8ac04657 >> +TVHEADEND_VERSION = v4.1-2345-g04ff649 > > Please keep this as a full hash, like it used to be. It should still May I ask for reason? Just for my curiosity... > work fine with the version reporting, if you write this version to > $(@D)/rpm/version, like you're doing below. Are you suggesting here that tvheadend about page should just report “50a370707aedf5c127e92bb517c378aa8ac04657” as a version?
Hello, On Mon, 28 Nov 2016 22:08:04 +0100, Damjan Marion wrote: > > Could you split the bump and the version fix reporting in two separate > > patches? > > ack Thanks! > >> -TVHEADEND_VERSION = 50a370707aedf5c127e92bb517c378aa8ac04657 > >> +TVHEADEND_VERSION = v4.1-2345-g04ff649 > > > > Please keep this as a full hash, like it used to be. It should still > > May I ask for reason? Just for my curiosity... Because we're using a full hash in every other package fetched from Git in Buildroot, and we like consistency :) > > work fine with the version reporting, if you write this version to > > $(@D)/rpm/version, like you're doing below. > > Are you suggesting here that tvheadend about page should just report > “50a370707aedf5c127e92bb517c378aa8ac04657” as a version? Yes, exactly. Thanks! Thomas
> On 28 Nov 2016, at 22:13, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > > Hello, > > On Mon, 28 Nov 2016 22:08:04 +0100, Damjan Marion wrote: > >>> Could you split the bump and the version fix reporting in two separate >>> patches? >> >> ack > > Thanks! > >>>> -TVHEADEND_VERSION = 50a370707aedf5c127e92bb517c378aa8ac04657 >>>> +TVHEADEND_VERSION = v4.1-2345-g04ff649 >>> >>> Please keep this as a full hash, like it used to be. It should still >> >> May I ask for reason? Just for my curiosity... > > Because we're using a full hash in every other package fetched from Git > in Buildroot, and we like consistency :) Unfortunately that was only meaningful way (I’m aware of) to provide some human-readable version information out from their repo. > >>> work fine with the version reporting, if you write this version to >>> $(@D)/rpm/version, like you're doing below. >> >> Are you suggesting here that tvheadend about page should just report >> “50a370707aedf5c127e92bb517c378aa8ac04657” as a version? > > Yes, exactly. I don't see how is this improvement compared to “0.0.0~unknown”. I really doubt that it is worth time to submit that as a patch…. What about something like: TVHEADEND_VERSION = 50a370707aedf5c127e92bb517c378aa8ac04657 TVHEADEND_PRETTY_VERSION = v4.1-2345-g50a3707 Thanks, Damjan
Hello, On Mon, 28 Nov 2016 22:24:20 +0100, Damjan Marion wrote: > > Because we're using a full hash in every other package fetched from Git > > in Buildroot, and we like consistency :) > > Unfortunately that was only meaningful way (I’m aware of) to provide some > human-readable version information out from their repo. Well, the full hash is by far the most precise and exact information. Anyone having the Git repository can do "git checkout <hash>" and get straight to the exact same commit. > I don't see how is this improvement compared to “0.0.0~unknown”. I really doubt > that it is worth time to submit that as a patch…. Well, 0.0.0~unknown doesn't say anything about the version. The full hash says it all. > What about something like: > > TVHEADEND_VERSION = 50a370707aedf5c127e92bb517c378aa8ac04657 > TVHEADEND_PRETTY_VERSION = v4.1-2345-g50a3707 Which have to be maintained separately :/ Thomas
> On 28 Nov 2016, at 22:43, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > > Hello, > > On Mon, 28 Nov 2016 22:24:20 +0100, Damjan Marion wrote: > >>> Because we're using a full hash in every other package fetched from Git >>> in Buildroot, and we like consistency :) >> >> Unfortunately that was only meaningful way (I’m aware of) to provide some >> human-readable version information out from their repo. > > Well, the full hash is by far the most precise and exact information. > Anyone having the Git repository can do "git checkout <hash>" and get > straight to the exact same commit. > >> I don't see how is this improvement compared to “0.0.0~unknown”. I really doubt >> that it is worth time to submit that as a patch…. > > Well, 0.0.0~unknown doesn't say anything about the version. The full > hash says it all. Yeah, assuming that end user understands that this string of “random” characters is really a version. > >> What about something like: >> >> TVHEADEND_VERSION = 50a370707aedf5c127e92bb517c378aa8ac04657 >> TVHEADEND_PRETTY_VERSION = v4.1-2345-g50a3707 > > Which have to be maintained separately :/ ok, anyway, let’s forget about this thread…. I can maintain it in my external tree.
On 28-11-16 22:03, Thomas Petazzoni wrote: > Hello, > > On Mon, 28 Nov 2016 11:32:31 +0100, Damjan Marion wrote: >> tvheadedend tracks version information based on git version >> numbers. As tvheadend is downloaded as tarball form github >> it was reporting version 0.0.0~unknown. >> >> Signed-off-by: Damjan Marion <damjan.marion@gmail.com> > > Could you split the bump and the version fix reporting in two separate > patches? > >> -TVHEADEND_VERSION = 50a370707aedf5c127e92bb517c378aa8ac04657 >> +TVHEADEND_VERSION = v4.1-2345-g04ff649 > > Please keep this as a full hash, like it used to be. It should still > work fine with the version reporting, if you write this version to > $(@D)/rpm/version, like you're doing below. I'd like to challenge this. We prefer hashes instead of tags because tags may still be changed while a hash is guaranteed stable. However, the same can be said of tarballs. For tarballs we have the hashes to detect such a situation, for VCS downloads we can do the same (if we check hashes). In this particular case, however, it's a github-helper URL so it is a stable tarball with a hash, so there is no reason not to use a tag. Here, it's not actually a tag, but rather than a 'describe' on top of a tag. Still, it uniquely identifies a commit, and it's more descriptive than just the sha1. For example, with the describe, you can immediately see between two versions which one is the most recent one[1]. So I do think there is value in providing a 'describe' version rather than just a sha1. There's one issue, however. v4.1-2345-g04ff649 works now because 04ff649 is a unique abbreviation. But when later another commit is generated that starts with 04ff649, you need to use v4.1-2345-g04ff649a. So I guess it should be v4.1-2345-g04ff649a4c13229ebcaa9828fa04339a49842dd5. Regards, Arnout [1] Well, not entirely, because v4.1.10 may be more recent than 4.2.6, but still it's more descriptive.
Hi Arnout, On Wed, Nov 30, 2016 at 12:28:39AM +0100, Arnout Vandecappelle wrote: > On 28-11-16 22:03, Thomas Petazzoni wrote: > > On Mon, 28 Nov 2016 11:32:31 +0100, Damjan Marion wrote: > >> -TVHEADEND_VERSION = 50a370707aedf5c127e92bb517c378aa8ac04657 > >> +TVHEADEND_VERSION = v4.1-2345-g04ff649 > > > > Please keep this as a full hash, like it used to be. It should still > > work fine with the version reporting, if you write this version to > > $(@D)/rpm/version, like you're doing below. > > I'd like to challenge this. We prefer hashes instead of tags because tags may > still be changed while a hash is guaranteed stable. However, the same can be > said of tarballs. For tarballs we have the hashes to detect such a situation, > for VCS downloads we can do the same (if we check hashes). I don't think we prefer hashes over tags. We do prefer full hashes over shortened ones. But tags are most preferable, and we have lots of git tags in <PKG>_VERSION. > In this particular case, however, it's a github-helper URL so it is a stable > tarball with a hash, so there is no reason not to use a tag. > > Here, it's not actually a tag, but rather than a 'describe' on top of a tag. > Still, it uniquely identifies a commit, and it's more descriptive than just the > sha1. For example, with the describe, you can immediately see between two > versions which one is the most recent one[1]. So I do think there is value in > providing a 'describe' version rather than just a sha1. I beg to deffer. In my opinion, v4.1-2345-g04ff649 is misleading because it gives the impression of a version close to v4.1, which is far from reality. > There's one issue, however. v4.1-2345-g04ff649 works now because 04ff649 is a > unique abbreviation. But when later another commit is generated that starts with > 04ff649, you need to use v4.1-2345-g04ff649a. So I guess it should be > v4.1-2345-g04ff649a4c13229ebcaa9828fa04339a49842dd5. That's better than v4.1-2345-g04ff649, but I think that leaving the 'v4.1-2345-g' prefix out is better yet. baruch
diff --git a/package/tvheadend/tvheadend.hash b/package/tvheadend/tvheadend.hash index ca10dc3..53cbccc 100644 --- a/package/tvheadend/tvheadend.hash +++ b/package/tvheadend/tvheadend.hash @@ -1,2 +1,2 @@ # Locally computed -sha256 71dd3d8efa32d592342ff6955cc2cd317f07e74d2d546977eb9bc45136dd5de6 tvheadend-50a370707aedf5c127e92bb517c378aa8ac04657.tar.gz +sha256 9af13b3e0381fa1f229eaf06acd8ad0d92013c2004a8005f1c66e56cd6d3a4d1 tvheadend-v4.1-2345-g04ff649.tar.gz diff --git a/package/tvheadend/tvheadend.mk b/package/tvheadend/tvheadend.mk index a3afec8..ad5d72d 100644 --- a/package/tvheadend/tvheadend.mk +++ b/package/tvheadend/tvheadend.mk @@ -4,7 +4,7 @@ # ################################################################################ -TVHEADEND_VERSION = 50a370707aedf5c127e92bb517c378aa8ac04657 +TVHEADEND_VERSION = v4.1-2345-g04ff649 TVHEADEND_SITE = $(call github,tvheadend,tvheadend,$(TVHEADEND_VERSION)) TVHEADEND_LICENSE = GPLv3+ TVHEADEND_LICENSE_FILES = LICENSE.md @@ -88,6 +88,7 @@ define TVHEADEND_CONFIGURE_CMDS endef define TVHEADEND_BUILD_CMDS + @echo $(TVHEADEND_VERSION) > $(@D)/rpm/version $(TARGET_MAKE_ENV) $(MAKE) -C $(@D) endef
tvheadedend tracks version information based on git version numbers. As tvheadend is downloaded as tarball form github it was reporting version 0.0.0~unknown. Signed-off-by: Damjan Marion <damjan.marion@gmail.com> --- package/tvheadend/tvheadend.hash | 2 +- package/tvheadend/tvheadend.mk | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-)