| Message ID | 20170713221924.12021-1-yann.morin.1998@free.fr |
|---|---|
| State | Changes Requested |
| Headers | show |
Yann, On 07/13/2017 03:19 PM, Yann E. MORIN wrote: > diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk > index accf48c464..f285395267 100644 > --- a/package/pkg-utils.mk > +++ b/package/pkg-utils.mk > @@ -86,9 +86,12 @@ endef > define legal-license-file # pkgname, pkgname-pkgver, pkgdir, filename, file-fullpath, {HOST|TARGET} > mkdir -p $(LICENSE_FILES_DIR_$(6))/$(2)/$(dir $(4)) && \ > { \ > - support/download/check-hash $(3)/$(1).hash $(5) $(4); \ > - ret=$${?}; \ > - case $${ret} in (0|3) ;; (*) exit 1;; esac; \ > + if [ -d $(3)/$($(PKG)_VERSION) ]; then \ > + support/download/check-hash $(3)/$($(PKG)_VERSION)/$(1).hash $(5) $(4); \ > + else \ > + support/download/check-hash $(3)/$(1).hash $(5) $(4); \ > + fi; \ > + case $${?} in (0|3) ;; (*) exit 1;; esac; \ > } && \ > cp $(5) $(LICENSE_FILES_DIR_$(6))/$(2)/$(4) > endef > I think this has unintended side effects. If there is already a $(PKG)_VERSION directory for patches, it will look for the hash file there even if there are no hash file differences, which results in missing license file hash. I believe it would work if you test -f on the actual .hash file instead of just the directory. But, do note that if there are multiple license files per package, and only one of them is different, this still results in putting all hashes including duplicate ones in separate hash files. Josh
Hi Yann, Joshua, On 14/07/2017 01:32, Joshua Henderson wrote: > Yann, > > On 07/13/2017 03:19 PM, Yann E. MORIN wrote: > >> diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk >> index accf48c464..f285395267 100644 >> --- a/package/pkg-utils.mk >> +++ b/package/pkg-utils.mk >> @@ -86,9 +86,12 @@ endef >> define legal-license-file # pkgname, pkgname-pkgver, pkgdir, filename, file-fullpath, {HOST|TARGET} >> mkdir -p $(LICENSE_FILES_DIR_$(6))/$(2)/$(dir $(4)) && \ >> { \ >> - support/download/check-hash $(3)/$(1).hash $(5) $(4); \ >> - ret=$${?}; \ >> - case $${ret} in (0|3) ;; (*) exit 1;; esac; \ >> + if [ -d $(3)/$($(PKG)_VERSION) ]; then \ >> + support/download/check-hash $(3)/$($(PKG)_VERSION)/$(1).hash $(5) $(4); \ >> + else \ >> + support/download/check-hash $(3)/$(1).hash $(5) $(4); \ >> + fi; \ >> + case $${?} in (0|3) ;; (*) exit 1;; esac; \ >> } && \ >> cp $(5) $(LICENSE_FILES_DIR_$(6))/$(2)/$(4) >> endef >> > > I think this has unintended side effects. If there is already a $(PKG)_VERSION directory for patches, > it will look for the hash file there even if there are no hash file differences, which results in missing > license file hash. > > I believe it would work if you test -f on the actual .hash file instead of just the directory. But, do note > that if there are multiple license files per package, and only one of them is different, this still results > in putting all hashes including duplicate ones in separate hash files. I agree with Josh here. A cleaner solution would be to put only version-bound hashes in version-named subdirs, and leave all other hashes in the base directory. This would remove all duplication and avoid unintended disabling of all hashes when one <version>/*.hash file is created, which would happen only for those versions that actually have that file. I think this can be achieved in a simple way by cat-ing together $(3)/$(1).hash and $(3)/$($(PKG)_VERSION)/$(1).hash in a temporary file and feed that to check-hash. Additionally if check-hash were able to read the hashes from stdin, no temporary file would be even needed, but this is not necessarily a good idea.
Joshua, All, On 2017-07-13 16:32 -0700, Joshua Henderson spake thusly: > On 07/13/2017 03:19 PM, Yann E. MORIN wrote: > > > diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk > > index accf48c464..f285395267 100644 > > --- a/package/pkg-utils.mk > > +++ b/package/pkg-utils.mk > > @@ -86,9 +86,12 @@ endef > > define legal-license-file # pkgname, pkgname-pkgver, pkgdir, filename, file-fullpath, {HOST|TARGET} > > mkdir -p $(LICENSE_FILES_DIR_$(6))/$(2)/$(dir $(4)) && \ > > { \ > > - support/download/check-hash $(3)/$(1).hash $(5) $(4); \ > > - ret=$${?}; \ > > - case $${ret} in (0|3) ;; (*) exit 1;; esac; \ > > + if [ -d $(3)/$($(PKG)_VERSION) ]; then \ > > + support/download/check-hash $(3)/$($(PKG)_VERSION)/$(1).hash $(5) $(4); \ > > + else \ > > + support/download/check-hash $(3)/$(1).hash $(5) $(4); \ > > + fi; \ > > + case $${?} in (0|3) ;; (*) exit 1;; esac; \ > > } && \ > > cp $(5) $(LICENSE_FILES_DIR_$(6))/$(2)/$(4) > > endef > > > > I think this has unintended side effects. If there is already a $(PKG)_VERSION directory for patches, > it will look for the hash file there even if there are no hash file differences, which results in missing > license file hash. This is the intended behaviour, yes: if there is a version choice, then we _want_ the license hashes to be per version, even if they are the same across versions. We do not want them in the 'main' hash file. Yes, this means duplicated entries. I was also wondering if we should also move the hashes for the downlaoded files to be per-version as well, to keep things together. Ie., we would have something like: package/qt5/qt5base/Config.in package/qt5/qt5base/qt5base.mk package/qt5/qt5base/5.6.2/*.patch package/qt5/qt5base/5.6.2/qt5base.hash package/qt5/qt5base/5.8.0/*.patch package/qt5/qt5base/5.8.0/qt5base.hash > I believe it would work if you test -f on the actual .hash file instead of just the directory. Again, if the directory exists, I intended that the main hash file was not used at all. > But, do note > that if there are multiple license files per package, and only one of them is different, this still results > in putting all hashes including duplicate ones in separate hash files. Yes, this is what I had in mind in this case. Regards, Yann E. MORIN.
Luca, All, On 2017-07-14 09:39 +0200, Luca Ceresoli spake thusly: > On 14/07/2017 01:32, Joshua Henderson wrote: > > Yann, > > > > On 07/13/2017 03:19 PM, Yann E. MORIN wrote: > > > >> diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk > >> index accf48c464..f285395267 100644 > >> --- a/package/pkg-utils.mk > >> +++ b/package/pkg-utils.mk > >> @@ -86,9 +86,12 @@ endef > >> define legal-license-file # pkgname, pkgname-pkgver, pkgdir, filename, file-fullpath, {HOST|TARGET} > >> mkdir -p $(LICENSE_FILES_DIR_$(6))/$(2)/$(dir $(4)) && \ > >> { \ > >> - support/download/check-hash $(3)/$(1).hash $(5) $(4); \ > >> - ret=$${?}; \ > >> - case $${ret} in (0|3) ;; (*) exit 1;; esac; \ > >> + if [ -d $(3)/$($(PKG)_VERSION) ]; then \ > >> + support/download/check-hash $(3)/$($(PKG)_VERSION)/$(1).hash $(5) $(4); \ > >> + else \ > >> + support/download/check-hash $(3)/$(1).hash $(5) $(4); \ > >> + fi; \ > >> + case $${?} in (0|3) ;; (*) exit 1;; esac; \ > >> } && \ > >> cp $(5) $(LICENSE_FILES_DIR_$(6))/$(2)/$(4) > >> endef > >> > > > > I think this has unintended side effects. If there is already a $(PKG)_VERSION directory for patches, > > it will look for the hash file there even if there are no hash file differences, which results in missing > > license file hash. > > > > I believe it would work if you test -f on the actual .hash file instead of just the directory. But, do note > > that if there are multiple license files per package, and only one of them is different, this still results > > in putting all hashes including duplicate ones in separate hash files. > > I agree with Josh here. > > A cleaner solution would be to put only version-bound hashes in > version-named subdirs, and leave all other hashes in the base directory. > This would remove all duplication and avoid unintended disabling of all > hashes when one <version>/*.hash file is created, which would happen > only for those versions that actually have that file. See my reply to Joshua: I would favour the opposite, in fact, to move all hashes to the per-version directory, even if there is duplication. > I think this can be achieved in a simple way by cat-ing together > $(3)/$(1).hash and $(3)/$($(PKG)_VERSION)/$(1).hash in a temporary file > and feed that to check-hash. Additionally if check-hash were able to > read the hashes from stdin, no temporary file would be even needed, but > this is not necessarily a good idea. cat-ing to a temporary file will bneed that we arrange for a trickier code, because we need a unique filename for when we want to support TLPB. As for stdin, this would be doable quite easily, we just need to recognise '-' as stdin, like almost all of the rest of the world. ;-) But still, my preference would be to move hashes to the per-version directory. All hashes, not just license ones. In the meantime, I've marked the patch as 'changes requested'. Regards, Yann E. MORIN.
On 14-07-17 10:38, Yann E. MORIN wrote: > Joshua, All, > > On 2017-07-13 16:32 -0700, Joshua Henderson spake thusly: >> On 07/13/2017 03:19 PM, Yann E. MORIN wrote: >> >>> diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk >>> index accf48c464..f285395267 100644 >>> --- a/package/pkg-utils.mk >>> +++ b/package/pkg-utils.mk >>> @@ -86,9 +86,12 @@ endef >>> define legal-license-file # pkgname, pkgname-pkgver, pkgdir, filename, file-fullpath, {HOST|TARGET} >>> mkdir -p $(LICENSE_FILES_DIR_$(6))/$(2)/$(dir $(4)) && \ >>> { \ >>> - support/download/check-hash $(3)/$(1).hash $(5) $(4); \ >>> - ret=$${?}; \ >>> - case $${ret} in (0|3) ;; (*) exit 1;; esac; \ >>> + if [ -d $(3)/$($(PKG)_VERSION) ]; then \ >>> + support/download/check-hash $(3)/$($(PKG)_VERSION)/$(1).hash $(5) $(4); \ >>> + else \ >>> + support/download/check-hash $(3)/$(1).hash $(5) $(4); \ >>> + fi; \ >>> + case $${?} in (0|3) ;; (*) exit 1;; esac; \ >>> } && \ >>> cp $(5) $(LICENSE_FILES_DIR_$(6))/$(2)/$(4) >>> endef >>> >> >> I think this has unintended side effects. If there is already a $(PKG)_VERSION directory for patches, >> it will look for the hash file there even if there are no hash file differences, which results in missing >> license file hash. > > This is the intended behaviour, yes: if there is a version choice, then > we _want_ the license hashes to be per version, even if they are the > same across versions. We do not want them in the 'main' hash file. > > Yes, this means duplicated entries. > > I was also wondering if we should also move the hashes for the > downlaoded files to be per-version as well, to keep things together. > > Ie., we would have something like: > > package/qt5/qt5base/Config.in > package/qt5/qt5base/qt5base.mk > package/qt5/qt5base/5.6.2/*.patch > package/qt5/qt5base/5.6.2/qt5base.hash > package/qt5/qt5base/5.8.0/*.patch > package/qt5/qt5base/5.8.0/qt5base.hash This indeed sounds like the right approach. But then both uses of the hash file (download and legal-info) should use the same approach, as you say. In fact, this can be a workaround for cases where we often don't have a hash because of custom version specification, like the kernel... Perhaps the nicest approach is to move the intelligence down into check-hash itself (the less shell code in the makefiles the better). So check-hash would not get a hash file as argument, but it would get package directory and version. Note by the way that pkg-stats should probably be updated as well. Sorry to give you more work :-) Regards, Arnout > > > >> I believe it would work if you test -f on the actual .hash file instead of just the directory. > > Again, if the directory exists, I intended that the main hash file was > not used at all. > >> But, do note >> that if there are multiple license files per package, and only one of them is different, this still results >> in putting all hashes including duplicate ones in separate hash files. > > Yes, this is what I had in mind in this case. > > Regards, > Yann E. MORIN. >
diff --git a/docs/manual/adding-packages-directory.txt b/docs/manual/adding-packages-directory.txt index 804946c504..809cc97389 100644 --- a/docs/manual/adding-packages-directory.txt +++ b/docs/manual/adding-packages-directory.txt @@ -482,7 +482,9 @@ this in a comment line above the hashes. .Note The hashes for license files are used to detect a license change when a -package version is bumped. +package version is bumped. For a package with multiple versions (like Qt5), +create the hash file in a subdirectory +<packageversion>+ of that package +(see also xref:patch-apply-order[]). .Note The number of spaces does not matter, so one can use spaces (or tabs) to diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk index accf48c464..f285395267 100644 --- a/package/pkg-utils.mk +++ b/package/pkg-utils.mk @@ -86,9 +86,12 @@ endef define legal-license-file # pkgname, pkgname-pkgver, pkgdir, filename, file-fullpath, {HOST|TARGET} mkdir -p $(LICENSE_FILES_DIR_$(6))/$(2)/$(dir $(4)) && \ { \ - support/download/check-hash $(3)/$(1).hash $(5) $(4); \ - ret=$${?}; \ - case $${ret} in (0|3) ;; (*) exit 1;; esac; \ + if [ -d $(3)/$($(PKG)_VERSION) ]; then \ + support/download/check-hash $(3)/$($(PKG)_VERSION)/$(1).hash $(5) $(4); \ + else \ + support/download/check-hash $(3)/$(1).hash $(5) $(4); \ + fi; \ + case $${?} in (0|3) ;; (*) exit 1;; esac; \ } && \ cp $(5) $(LICENSE_FILES_DIR_$(6))/$(2)/$(4) endef
When we have multiple versions for a package, and the licensing terms depend on the version actually selected (e.g. like Qt5), storing the hashes for those license files in the .hash file is broken: the infra will ensure that all hashes for a file do match, which would not be the case here. We fix that by first looking for a hash file in the version sub-dir first, and if that directory does not exist, then we use the main hash file. Note that if the version sub-dir exists, the main hash file is not used even if there is no per-version hash file. Update the documentation accordingly. Reported-by: Joshua Henderson <joshua.henderson@microchip.com> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr> Cc: Joshua Henderson <joshua.henderson@microchip.com> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> Cc: Luca Ceresoli <luca@lucaceresoli.net> --- docs/manual/adding-packages-directory.txt | 4 +++- package/pkg-utils.mk | 9 ++++++--- 2 files changed, 9 insertions(+), 4 deletions(-)