Re: [Buildroot] [PATCH] package/pkg-generic: store reall version in legal manifest

[Luca Ceresoli via buildroot <buildroot@buildroot.org>]

Hi Yann,

On Tue, 21 Jan 2025 11:45:32 +0100
<yann.morin@orange.com> wrote:

> From: "Yann E. MORIN" <yann.morin@orange.com>
> 
> The legal manifest currently stores the $(PKG)_VERSION variable.
> However, that variable undergoes a set of changes so that it is
> suitable for creating files and Makefile rules; that new value
> is purely a technical, internal detail of how Buildroot handles
> things.
> 
> In the legal manifest, we need access to the real value for the
> version, as this is what will allow actual references to the
> upstream package. If the version string is mangled, like slashes
> replaced with underscores, this introduces ambiguities as to what
> exactly the version is.
> 
> Change the legal manifest to include the actual, original value as
> was set in the .mk file.
> 
> Signed-off-by: Yann E. MORIN <yann.morin@orange.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> Cc: Luca Ceresoli <luca.ceresoli@bootlin.com>

Subject line: reall -> real.

It would be nice to shot an actual example in the commit message. I did
a very quick search and couldn't find any package where the problem
happens.

So if there is an upstream package affected I'd list that, otherwise
you can fake an example, as I did to test your patch, e.g.:

  For a package having:

    FOO_VERSION = "12/34"

  the version in the manifest actually is "12_34". With this patch it
  will be "12/34" as expected.

> -	@$$(call legal-manifest,$$(call UPPERCASE,$(4)),$$($(2)_RAWNAME),$$($(2)_VERSION),$$(subst $$(space)$$(comma),$$(comma),$$($(2)_LICENSE)),$$($(2)_MANIFEST_LICENSE_FILES),$$($(2)_ACTUAL_SOURCE_TARBALL),$$($(2)_ACTUAL_SOURCE_SITE),$$(call legal-deps,$(1)))
> +	@$$(call legal-manifest,$$(call UPPERCASE,$(4)),$$($(2)_RAWNAME),$$($(2)_DL_VERSION),$$(subst $$(space)$$(comma),$$(comma),$$($(2)_LICENSE)),$$($(2)_MANIFEST_LICENSE_FILES),$$($(2)_ACTUAL_SOURCE_TARBALL),$$($(2)_ACTUAL_SOURCE_SITE),$$(call legal-deps,$(1)))
>  endif # ifneq ($$(call qstrip,$$($(2)_SOURCE)),)
>  	$$(foreach hook,$$($(2)_POST_LEGAL_INFO_HOOKS),$$(call $$(hook))$$(sep))

Not strictly specific about your patch, but IMO reusing the _DL_VERSION
variable makes code less readable. The _DL_VERSION name suggests it is
for downloading, right? What about renaming it to e.g. _ORIG_VERSION,
and then using _ORIG_VERSION for both the download, the legal manifest,
pkg-stats and whatever use is currently made of _DL_VERSION?

Other than that looks good, and I'm not strict about the _DL_VERSION so:

Tested-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot