legal-info improvements and completeness (branch yem/legal-3)

Hello All!

This series brings improvements to the legal-info infrastructure, so that we provide the most complete and correct content in the output of legal-info.

Currently, our legal-info ouput is missing two types of files that might be important to have:
- patches
- extra downloads

This series is split in 6 consecutive parts, each depending on the previous ones:

- patches 1 adds hashes to all actual sources for pre-built toolchains

- patches 2-5 make sure legal-info will work in off-line mode;

- pathces 6-8 reorganise the legal-info directory structure to accomodate for the fact that more than one source archive/file may be saved for each package;

- patches 9-12 actually save the patches and extra downloads in the

    legal-info output;

- patch 13 adds a list of hashes for all files in the legal-info

    output;

- patches 14-15 add the possibility to ignore packages from the

    legal-info output; this is then used to ignore virtual packages;

- patch 16 (from Luca) explicits patches licensing.

Why save patches?
-----------------

So far, we've shuffled the patches under the rag, assuming the user would provide the Buildroot source tree with the compliance delivery, so that our bundled patches would automatically be included.

However, that's not enough, as not all patches may be in the Buildroot source tree. That's the case for at least two types of patches:
- patches that are downloaded,
- patches from a global patch directory.

In either case, those patches must be provided in the output of legal-info, because they are not part of Buildroot, so distributing Buidlroot would not be enough.

Patches that are referenced from Buidlroot (like patches retrieved at download time from a http://-or-such scheme to a publicly-reachable location) would probably be OK-ish, even if not to the letter of the compliance requirements.

That's not so much the case for patches from a global patch dir, since those would be completely ignored and usually unreachable from a recipient of the compliance delivery.

So we must save those two types of patches in the output of legal-info. Because it would be a bit silly to only save the non-bundled patches, we just save all of them, whether bundled in Buildroot, downloaded or from a global patch dir alike.

Note about saving patches
-------------------------

Buildroot can apply patches from at least three different locations:
- patches bundled in Buildroot itself,
- patches download either with FOO_PATCH of FOO_EXTRA_DOWNLOAD
- patches from one or more BR2_GLOBAL_PATCH_DIR

Since those patches are stored in three different locations, it is perfectly legit that two patches in two different locations have the same basename.

However, when saving the patches, the second to be saved would overwrite the previous one, and would thus cause troubles down the road:

- the second patch would be applied earlier due to it being referenced by the series file, and thus may not apply; even if it applies cleanly, it would still be listed a second time in the series, and thus would fail to apply that second time,

- the compliance distribution would not longer be compliant

Hence we have two options at our disposal:

- rename the patch files so no two patches have the same name; this is easily achieved by prefixing them with a monotonically-incremented counter, or

- detect that not two patches have the same basename and fail at apply-patch time if that is the case.

This series implements the first solution. The immediate drawback is that most patches will have two indexes, while the immediate advantage is that it accepts input patches with the same basename.

The alternative second solution, although tested and implemented (but not submitted) has an immediate drawback of breaking existing working setups with patches with the same basename, while there is no real advantage, except not double-numbering the patches for legal-info.

Why save extra downloads?
-------------------------

Some packages are using extra-downloads to complement the content of the main archive. That's the case for Perl, for which the cross-compilation "enabler" is downloaded as a secondary archive via extra downloads. The Blackfin external toolchains also use extra downloads to download a secondary archive with the sysroot.

Even though the Blackfin sysroot archive is not really a source, we still need to provide it along with the main archive, otherwise it's completely impossible to compile with just the "main" toolchain.

As for the Perl case, however, we're "only" downloading a buildsystem infrastructure (AFAIU), but without it, it is completely impossible to cross-compile Perl at all.

So, in both cases, we also need to save the extra downloads.

Changes v4 -> v5:
- fix the macro by making it ashell script (Luca)
- typoes

Changes v3 -> v4:
- add hashes for sources to more CodeSourcery pre-built toolchains (Arnout)
- fix hardlink-or-copy when the destination may already exist (Arnout, Luca)
- handle downloading actual sources with a stamp file (Luca)
- rephrase manual about ignoring packages (Luca)
- typoes (Luca)
- add first patch, to fix fetching sources for Linaro pre-built toolchains

Changes v2 -> v3:
- re-order variables in their own patch (Arnout)
- update legal-info header about the patches (Luca)
- add hashes for external toolchains sources (Luca, Arnout)
- misc and typoes (Arnout, Luca)
- enhance the hardlink-or-copy macro

Changes v1 -> v2:
- keep only the core legal-info patches, drop the gcc/binutils/gdb changes (they'll be reworked later, let's focus on the important and easier parts first)
- drop the tristate REDISTRIBUTE, introduce another boolean _LEGAL_IGNORE (Thomas, Peter, Luca)
- drop the post-legal-info Perl hook, it's no longer needed thanks to saving extra downloads (Thomas, Luca)
- compute the rawname-version tuple only once, instead of five times

    (Luca)

- reorder patches (Luca)
- slight commit log rephrasing and corrections (Luca)

Regards,
Yann E. MORIN.

The following changes since commit 0b7b84310c3781c346eea4c114c8cb4e97bbd8ea:

olimex_imx233_olinuxino_defconfig: genimage support (2016-03-11 13:14:54 +0100)

are available in the git repository at:

git://git.busybox.net/~ymorin/git/buildroot yem/legal-3

for you to fetch changes up to d6c5a79915775eaa9d1a6d1711a4955cafe221cd:

legal-info: explicitly state how patches are licensed (2016-03-11 18:11:54 +0100)

----------------------------------------------------------------
Luca Ceresoli (1):
legal-info: explicitly state how patches are licensed

Yann E. MORIN (15):
toolchain/external: add hashes for actual sources core/pkg-utils: add macro to hardlink-or-copy core/legal-info: use the macro to install source archives core/pkg-generic: reorder variables definitions for legal-info core/legal-info: ensure legal-info works in off-line mode core/pkg-generic: add variable to store the package rawname-version core/legal-info: install source archives in their own sub-dir core/legal-info: add package version to license directory core/apply-patches: store full path of applied patches core/legal-info: also save patches
core/legal-info: renumber saved patches core/legal-info: also save extra downloads core/legal-info: generate a hash of all saved files core/legal-info: allow ignoring packages from the legal-info core/pkg-virtual: ignore from legal-info output

 Makefile                                           |  8 +-
 docs/manual/adding-packages-generic.txt            |  9 +++
 docs/manual/adding-packages-virtual.txt            |  5 ++
 package/pkg-generic.mk                             | 94 +++++++++++++++++-----
 package/pkg-utils.mk                               | 22 +++++
 package/pkg-virtual.mk                             |  2 +
 support/legal-info/README.header                   |  9 ++-
 support/scripts/apply-patches.sh                   | 11 ++-
 support/scripts/hardlink-copy                      | 34 ++++++++
 .../toolchain-external/toolchain-external.hash     | 13 +++
 10 files changed, 177 insertions(+), 30 deletions(-)
 create mode 100755 support/scripts/hardlink-copy

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

As we currently download the actual sources as part of saving the legal-info, we do not check the hashes of those downloads.

That's because, during legal-info, there is not package involved, and thus there's no path to an actual .hash file.

However, this precludes legal-info from working in off-line mode. A subsequent patch will make it possible to do so, and actual sources will be downloaded as another classical package download.

This will have two consequences:

- first, we will be able to add hashes for actual sources, so we can ensure their integrity,

- second, and as a direct consequence of the above, when a .hash file is present, it would have to list all the hashes for that package, or that would be treated as an error.

Currently, the only package that falls in this case is the external- toolchain, for which we have means to retrieve the sources for some of the toolchains.

So we just add hashes for those actual external-toolchain sources we may have to download.

Those hashes are not used for now, but they'll come into play a few patches down.

---
 toolchain/toolchain-external/toolchain-external.hash | 13 +++++++++++++
 1 file changed, 13 insertions(+)

index ac50281..0ed253d 100644
@@ -12,30 +12,43 @@ sha256 c65b1b4b918d5185349d62a3b7bf43b4b21e1175c52598ec047ca56b3f11d857  blackfi
 # Mentor's Sourcery CodeBench Lite toolchains
 # ARM
 sha256 39ee0e789034334ecc89af94e838e3a4815400ac5ff980f808f466b04778532e  arm-2014.05-29-arm-none-linux-gnueabi-i686-pc-linux-gnu.tar.bz2
+sha256 e16a5b1e41d7ff1e74161f9405182001bc8d1360d89564e73911032e6966cc0d  arm-2014.05-29-arm-none-linux-gnueabi.src.tar.bz2
 # NiosII
 sha256 cc47745dc1264fcb8fb98fb1315ab772ab98691396021c455229b58abaf887f5  sourceryg++-2015.11-27-nios2-linux-gnu-i686-pc-linux-gnu.tar.bz2
+sha256 8b5710e41685a316d577b503c068c69ba1eaf228fe0a056cac36945323a0dd23  sourceryg++-2015.11-27-nios2-linux-gnu.src.tar.bz2
 # PowerPC
 sha256 525e1f53abbf65c2974ae9af762c45bb38520fe5fc50e968a23fe6a18e9eec04  freescale-2011.03-38-powerpc-linux-gnu-i686-pc-linux-gnu.tar.bz2
+sha256 fe45a1d2725e1b9f0ec7b8041c923e01f461da01b298caad537854ec1c61e211  freescale-2011.03-38-powerpc-linux-gnu.src.tar.bz2
 sha256 d6c94587d546197836e7e1a6909f6aabfa5879e91f501ab03088a6887cc242fc  mentor-2012.03-71-powerpc-mentor-linux-gnu-i686-pc-linux-gnu.tar.bz2
+sha256 4964938da0108a9a5bcc295b0f504a3804021e6e6e596509ee085d9143711b50  mentor-2012.03-71-powerpc-mentor-linux-gnu.src.tar.bz2
 # SuperH
 sha256 59d6766fde244931aa52db01433d5acd051998762a931121c5fc109536a1a802  renesas-2012.09-61-sh-linux-gnu-i686-pc-linux-gnu.tar.bz2
+sha256 e2e58c10e52395d5d35157e35f85233f713c6f9223a652dfc56194cfd2eed004  renesas-2012.09-61-sh-linux-gnu.src.tar.bz2
 # x86
 sha256 ea804cf02014369da52abc4f64e91e96bde2dd2230aca96109459013d4545458  ia32-2012.09-62-i686-pc-linux-gnu-i386-linux.tar.bz2
+sha256 1a9519e415a1e6892c760bf21f7e98f3a633a9d1c5bb8781a96d338e4dd62717  ia32-2012.09-62-i686-pc-linux-gnu.src.tar.bz2
 # AMD64
 sha256 cb4d071db8aefb8005fe72824b96568d93a50f5acd85bacf505a34fe2f265f70  amd-2015.11-36-x86_64-amd-linux-gnu-i686-pc-linux-gnu.tar.bz2
+sha256 a4de11669cddd4594f228db7bd38b0fed3c8692d1bac021739390b4c766bbd4f  amd-2015.11-36-x86_64-amd-linux-gnu.src.tar.bz2
 # Aarch64
 sha256 8ea78c5988b2bb507534f1ad46aa46659f66b39d55f2fc40e163a90b4195e70f  aarch64-2014.05-30-aarch64-linux-gnu-i686-pc-linux-gnu.tar.bz2
+sha256 fef1ba94b43f2aefe5634815745c1cb5442a12d12d98e653601392cb65517b7f  aarch64-2014.05-30-aarch64-linux-gnu.src.tar.bz2
 
 # ARM toolchains from Texas Instrument's Arago project
+# There is one source file that covers both binary distributions.
 sha256 f2febf3b3c565536461ad4405f1bcb835d75a6afb2a8bec958a1248cb4b81fc7  arago-2011.09-armv7a-linux-gnueabi-sdk.tar.bz2
 sha256 254af7d02eb3bcc8345c78e131700bc995d65b68232caaed21150a5fd1456070  arago-2011.09-armv5te-linux-gnueabi-sdk.tar.bz2
+sha256 25fbf0513ad7322b15cbaae964cafadcbb4c939f2708f57f40b8f9f2d601122b  arago-toolchain-2011.09-sources.tar.bz2
 
 # ARM and Aarch64 toolchains from Linaro
 sha256 0cffac0caea0eb3c8bdddfa14be011ce366680f40aeddbefc7cf23cb6d4f1891  gcc-linaro-arm-linux-gnueabihf-4.9-2014.09_linux.tar.xz
+sha256 eafeb3a5247e9ce31aa35d812e296fba5d5f1443e106d9bef9e38d3ee3ade006  gcc-linaro-arm-linux-gnueabihf-4.9-2014.09_src.tar.bz2
 sha256 ff2d231749e59968cb5e7032b4f4e4ae82ff9f11c23967863e627a6c59cb3bc0  gcc-linaro-5.2-2015.11-2-x86_64_arm-linux-gnueabihf.tar.xz
 sha256 4bc9d86390f8fa67a693ba4768ba5b12faaf7dd37c706c05ccd9321e765226e4  gcc-linaro-armeb-linux-gnueabihf-4.9-2014.09_linux.tar.xz
+sha256 bf5d3111dad5aa9aef0e955875fb7fc9e918cb24519af7014dd67a9e581a49b1  gcc-linaro-armeb-linux-gnueabihf-4.9-2014.09_src.tar.bz2
 sha256 e3df0c31d0bd8d1f6d45d585203c0f601e50b1ff7225da1423a8ca36e8caf58f  gcc-linaro-5.2-2015.11-2-x86_64_armeb-linux-gnueabihf.tar.xz
 sha256 3954f496ab01de67241109e82abfaa9b7625fdab4f05e79e7902e9814a07b832  gcc-linaro-aarch64-linux-gnu-4.9-2014.09_linux.tar.xz
+sha256 3954f496ab01de67241109e82abfaa9b7625fdab4f05e79e7902e9814a07b832  gcc-linaro-aarch64-linux-gnu-4.9-2014.09_src.tar.bz2
 sha256 b318a1837a54146b0120a13852386576e38355513b4e2cd5e2125f9c26913777  gcc-linaro-5.2-2015.11-2-x86_64_aarch64-linux-gnu.tar.xz
 
 # Codescape toolchains from Imagination Technologies

1.9.1

Hello,

Applied to master, thanks.

Thomas

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

This macro will try to copy a source file into a destination directory, by first attempting to hard-link, and falling back to a plain copy.

In some situations, it will be necessary that the destination file is named differently than the source (e.g. due to a re-numbering), so if a third argument is specified, it is treated as the basename of the destination file.

In future commits, this macro will be passed shell-level variables, e.g. in a for-loop, so we can't evaluate those at make-level. Thus, we delegate the full macro to a shell script. We still keep the make macro, as it is easier to write, and allows to $(strip) its arguments in a convenient fashion.

---
Changes v4 -> v5:
- move the body of the macro to a shell script (Luca)

Changes v3 -> v4:
- forcibly remove destination file first (Arnout, Luca)
- typoes (Luca)
- drop trailing slash in destination directory name

Changes v2 -> v3;
- use "ln" instead of "cp -l"
- accept third argument, as the basename of the destination file
- drop reviewed-by and tested-by tags given in v2, due to the above two changes

Changes RFC -> v1:
- move to pkg-utils (Luca)

---
 package/pkg-utils.mk          | 22 ++++++++++++++++++++++
 support/scripts/hardlink-copy | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)
 create mode 100755 support/scripts/hardlink-copy

index f88313a..a5155e8 100644
@@ -113,6 +113,28 @@ $$(error Package error: use $(2) instead of $(1). Please fix your .mk file)
 endif
 endef
 
+################################################################################
+# hardlink-copy -- hardlink source and destination if possible, otherwise
+# do a simple copy
+#
+# argument 1 is the source *file*
+# argument 2 is the destination *directory*
+# argument 3 is the basename of the destination file (optional, defaults to
+#            the basename of the source file.
+#
+# examples:
+#   $(call hardlink-copy,/path/to/source/file,/path/to/destination/dir)
+#   $(call hardlink-copy,/path/to/source/file,/path/to/destination/dir,new-name)
+#
+# Notes:
+#  - this is NOT an atomic operation,
+#  - this is only a wrapper to a shell script, so that it can be used with
+#    shell-level variables, like in a for loop.
+################################################################################
+define hardlink-copy
+	support/scripts/hardlink-copy "$(strip $(1))" "$(strip $(2))" "$(strip $(3))"
+endef
+
 #
 # legal-info helper functions
 #

new file mode 100755
index 0000000..9c1f883
@@ -0,0 +1,34 @@
+#!/bin/bash
+
+# Try to hardlink a file into a directory, fallback to copy on failure.
+#
+# Hardlink-or-copy the source file in the first argument into the
+# destination directory in the second argument, using the basename in
+# the third argument as basename for the destination file. If the third
+# argument is missing, use the basename of the basename of the source
+# file as basename for the destination file.
+#
+# In either case, remove the destination prior to doing the
+# hardlink-or-copy.
+#
+# Note that this is NOT an atomic operation.
+
+set -e
+
+main() {
+    local src_f="${1}"
+    local dst_d="${2}"
+    local dst_f="${3}"
+
+    if [ -n "${dst_f}" ]; then
+        dst_f="${dst_d}/${dst_f}"
+    else
+        dst_f="${dst_d}/$( basename "${src_f}" )"
+    fi
+
+    mkdir -p "${dst_d}"
+    rm -f "${dst_f}"
+    ln -f "${src_f}" "${dst_f}" 2>/dev/null || cp -f "${src_f}" "${dst_f}"
+}
+
+main "${@}"

1.9.1

Dear Yann E. MORIN,

	rm -f $(2)
	ln -f $(1) $(2) 2>/dev/null || cp -f $(1) $(2)

	$(call hardlink-or-copy-inner,$(1),$(if $(3),$(2)/$(3),$(2)/$(basename $(1))))

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

I disagree. This may be slightly shorter in lines of code, but I think the
shell script is much more readable.

However, I do wonder if it makes sense to have a make function for it if there
already is a shell script. We don't do that for apply-patches, so why would we do it for this one?

I agree with that.

Regards,
Arnout

Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

Arnout, All,

On 2016-03-19 17:08 +0100, Arnout Vandecappelle spake thusly:

And as explained in the commit log, it does not work if any of the argument needs shell-level expansion (see my other reply to Thomas).

As explained in the ocmmit log:
We still keep the make macro, as it is easier to [call], and allows to $(strip) its arguments in a convenient fashion.

Note: I now use "call" when the commit log used "write". What is easier is not to write the macro (well, it is, but that was not the point of that sentence), but writing code that calls the macro.

Also, I think it is better to have callers call a make macro, even if backe by a shell script. It is more "in-line" with how we do other stuff, like the github helper. Our helpers are make macros, it is more homogeneous.

Fixed.

Thanks! :-)

Regards,
Yann E. MORIN.

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

Hello,

What about the pure make based implementation?

Thomas

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[snip]

I honestly don't see why it is easier to call...

# Save the source tarball

	$$(Q)$$(HARDLINK_OR_COPY) \
		$$(DL_DIR)/$$($(2)_ACTUAL_SOURCE_TARBALL) \
		$$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4)))

# Save patches and generate the series file

	$$(Q)while read f; do \
		$$(HARDLINK_OR_COPY) $$$${f} $$($(2)_REDIST_SOURCES_DIR) \
			|| exit 1; \
		printf "%s\n" "$$$${f##*/}">>$$($(2)_REDIST_SOURCES_DIR)/series\
			|| exit 1; \
	done <$$($(2)_DIR)/.applied_patches_list

Why is this needed?

$(call ...) is ugly, there is no way you're going to convince me it's better
than simply calling a shell script. Oh, and the github helper should go :-)

Regards,
Arnout

[snip]

Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

Thomas, All,

On 2016-03-19 15:23 +0100, Thomas Petazzoni spake thusly:

Well, that does not work. That's basically what I did in the previous iteration, but in some cases, $(3) could be a shell expansion and it is not empty at the time the macro is called, but ends up being empty after the shell expansion. See the discussion we had with Luca about this:

    http://lists.busybox.net/pipermail/buildroot/2016-February/152634.html

OK, will change.

Thanks! :-)

Regards,
Yann E. MORIN.

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

Hello,

Hum, I see. We wouldn't have this problem if we were to know, at the infra level, the entire list of patches that get applied. But indeed, if we need to use the file generated by apply-patches, using a pure make function doesn't seem to work.

Thomas

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

.. and use $(Q) instead of a hard-coded @.

---
Changes v4 -> v5:
- s/Copy/Save/ because we're not really copying

Changes v2 -> v3:
- comment the @ -> $(Q) change (Arnout)

---
 package/pkg-generic.mk | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

index 3904c09..f47dc7e 100644
@@ -803,9 +803,10 @@ ifeq ($$($(2)_REDISTRIBUTE),YES)
 ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),$$($(2)_SOURCE))
 	$$(call DOWNLOAD,$$($(2)_ACTUAL_SOURCE_SITE)/$$($(2)_ACTUAL_SOURCE_TARBALL))
 endif
-# Copy the source tarball (just hardlink if possible)
-	@cp -l $$(DL_DIR)/$$($(2)_ACTUAL_SOURCE_TARBALL) $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4))) 2>/dev/null || \
-	    cp $$(DL_DIR)/$$($(2)_ACTUAL_SOURCE_TARBALL) $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4)))
+# Save the source tarball
+	$$(Q)$$(call hardlink-copy,\
+		     $$(DL_DIR)/$$($(2)_ACTUAL_SOURCE_TARBALL),\
+		     $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4))))
 endif # redistribute
 
 endif # other packages

1.9.1

Move the declarations of _ACTUAL_SOURCE and _ACTUAL_SITE earlier, so that they are close to where _SOURCE and _SITE are handled.

This looks so far like a purely cosmetic change, but makes more sense with the follow-up patch, where we'll need them earlier in the file.

---
Changes v2 -> v3:
- move that to its own patch (Arnout)

---
 package/pkg-generic.mk | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

index f47dc7e..8e70eae 100644
@@ -427,6 +427,14 @@ ifndef $(2)_SOURCE
  endif
 endif
 
+# If FOO_ACTUAL_SOURCE_TARBALL is explicitly defined, it means FOO_SOURCE is
+# indeed a binary (e.g. external toolchain) and FOO_ACTUAL_SOURCE_TARBALL/_SITE
+# point to the actual sources tarball. Use the actual sources for legal-info.
+# For most packages the FOO_SITE/FOO_SOURCE pair points to real source code,
+# so these are the defaults for FOO_ACTUAL_*.
+$(2)_ACTUAL_SOURCE_TARBALL ?= $$($(2)_SOURCE)
+$(2)_ACTUAL_SOURCE_SITE    ?= $$(call qstrip,$$($(2)_SITE))
+
 ifndef $(2)_PATCH
  ifdef $(3)_PATCH
   $(2)_PATCH = $$($(3)_PATCH)

@@ -762,14 +770,6 @@ $(1)-legal-info: $(1)-source $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4)))
 endif
 endif
 
-# If FOO_ACTUAL_SOURCE_TARBALL is explicitly defined, it means FOO_SOURCE is
-# indeed a binary (e.g. external toolchain) and FOO_ACTUAL_SOURCE_TARBALL/_SITE
-# point to the actual sources tarball. Use the actual sources for legal-info.
-# For most packages the FOO_SITE/FOO_SOURCE pair points to real source code,
-# so these are the defaults for FOO_ACTUAL_*.
-$(2)_ACTUAL_SOURCE_TARBALL ?= $$($(2)_SOURCE)
-$(2)_ACTUAL_SOURCE_SITE    ?= $$(call qstrip,$$($(2)_SITE))
-
 # legal-info: produce legally relevant info.
 $(1)-legal-info:
 # Packages without a source are assumed to be part of Buildroot, skip them.

1.9.1

Almost all packages which are saved for legal-info have their source archives downloaded as part of 'make source', which makes an off-line build completely possible [0].

However, for the pre-configured external toolchains, the source tarball is different, as the main tarball is a binary package. And that source tarball is only downloaded during the legal-info phase, which makes it inconvenient for full off-line builds.

We fix that by adding a new rule, $(1)-legal-source which only $(1)-all-source depends on, so that we only download it for a top-level 'make source', not as part of the standard download mechanism (i.e. only what is really needed to build).

This new rule depends, like the normal download mechanism, on a stamp file, so that we do not emit a spurious hash-check message on successive runs of 'make source'.

This way, we can do a complete [0] off-line build and are still able to generate legal-info, while at the same time we do not incur any download overhead during a simple build.

Also, we previously downloaded the _ACTUAL_SOURCE_TARBALL when it was not empty. However, since _ACTUAL_SOURCE_TARBALL defaults to the value of _SOURCE, it can not be empty when _SOURCE is not. Thus, we'd get a spurious report of a missing hash for the tarball, since it was not in a standard package rule (configure, build, install..) and thus would miss the PKG and PKGDIR variables to find the .hash file.

We fix that in this commit as well, by:

- setting PKG and PKGDIR just for the -legal-source rule;

- only downloading _ACTUAL_SOURCE_TARBALL if it is not empty *and* not the same as _SOURCE (to avoid a second report about the hash).

[0] Save for nodejs which invarriably wants to download stuff at build time. Sigh... :-( Fixing that is work for another time...

---
Changes v3 -> v4:
- handle it with a stamp file (Luca)

Changes v2 -> v3:
- re-order the PHONY targets (Arnout)
- don't reorder setting _ACTUAL_SOURCE/_SITE, it's done in its own patch now (Arnout)
- adapt the commit log accordingly (Arnout)
- typo

Changes v1 -> v2:
- drop the 'redistribute == ignore', it does not exist yet

---
 package/pkg-generic.mk | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

index 8e70eae..ce5525c 100644
@@ -123,6 +123,12 @@ $(BUILD_DIR)/%/.stamp_downloaded:
 	$(Q)mkdir -p $(@D)
 	$(Q)touch $@
 
+# Retrieve actual source archive, e.g. for prebuilt external toolchains
+$(BUILD_DIR)/%/.stamp_actual_downloaded:
+	$(call DOWNLOAD,$($(PKG)_ACTUAL_SOURCE_SITE)/$($(PKG)_ACTUAL_SOURCE_TARBALL)); \
+	$(Q)mkdir -p $(@D)
+	$(Q)touch $@
+
 # Unpack the archive
 $(BUILD_DIR)/%/.stamp_extracted:
 	@$(call step_start,extract)

@@ -527,6 +533,7 @@ $(2)_TARGET_RSYNC =	        $$($(2)_DIR)/.stamp_rsynced
 $(2)_TARGET_PATCH =		$$($(2)_DIR)/.stamp_patched
 $(2)_TARGET_EXTRACT =		$$($(2)_DIR)/.stamp_extracted
 $(2)_TARGET_SOURCE =		$$($(2)_DIR)/.stamp_downloaded
+$(2)_TARGET_ACTUAL_SOURCE =	$$($(2)_DIR)/.stamp_actual_downloaded
 $(2)_TARGET_DIRCLEAN =		$$($(2)_DIR)/.stamp_dircleaned
 
 # default extract command

@@ -634,6 +641,17 @@ $(1)-depends:		$$($(2)_FINAL_DEPENDENCIES)
 
 $(1)-source:		$$($(2)_TARGET_SOURCE)
 
+$(1)-all-source:	$(1)-legal-source
+$(1)-legal-info:	$(1)-legal-source
+$(1)-legal-source:	$(1)-source
+
+# Only download the actual source if it differs from the 'main' archive
+ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),)
+ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),$$($(2)_SOURCE))
+$(1)-legal-source:	$$($(2)_TARGET_ACTUAL_SOURCE)
+endif # actual sources != sources
+endif # actual sources != ""
+
 $(1)-source-check:
 	$$(foreach p,$$($(2)_ALL_DOWNLOADS),$$(call SOURCE_CHECK,$$(p))$$(sep))
 

@@ -659,6 +677,7 @@ $(1)-extract:		$(1)-rsync
 $(1)-rsync:		$$($(2)_TARGET_RSYNC)
 
 $(1)-source:
+$(1)-legal-source:
 
 $(1)-source-check:
 	test -d $$($(2)_OVERRIDE_SRCDIR)

@@ -733,6 +752,8 @@ $$($(2)_TARGET_PATCH):			PKGDIR=$(pkgdir)
 $$($(2)_TARGET_EXTRACT):		PKG=$(2)
 $$($(2)_TARGET_SOURCE):			PKG=$(2)
 $$($(2)_TARGET_SOURCE):			PKGDIR=$(pkgdir)
+$$($(2)_TARGET_ACTUAL_SOURCE):		PKG=$(2)
+$$($(2)_TARGET_ACTUAL_SOURCE):		PKGDIR=$(pkgdir)
 $$($(2)_TARGET_DIRCLEAN):		PKG=$(2)
 
 # Compute the name of the Kconfig option that correspond to the

@@ -800,9 +821,6 @@ else
 # Other packages
 
 ifeq ($$($(2)_REDISTRIBUTE),YES)
-ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),$$($(2)_SOURCE))
-	$$(call DOWNLOAD,$$($(2)_ACTUAL_SOURCE_SITE)/$$($(2)_ACTUAL_SOURCE_TARBALL))
-endif
 # Save the source tarball
 	$$(Q)$$(call hardlink-copy,\
 		     $$(DL_DIR)/$$($(2)_ACTUAL_SOURCE_TARBALL),\

@@ -897,6 +915,7 @@ endif
 	$(1)-install-staging \
 	$(1)-install-target \
 	$(1)-legal-info \
+	$(1)-legal-source \
 	$(1)-patch \
 	$(1)-rebuild \
 	$(1)-reconfigure \

1.9.1

Hello,

I am wondering what is the motivation for making "make legal-info" absolutely executable off-line, after a "make source". I was about to apply this patch, so my opinion is definitely not firm on this, but I have some concern:

* Now "make source" is downloading more stuff than is actually needed to do the build. And potentially a *lot* more this is going to download the source code for the toolchain, which you absolutely don't care about. So for a CodeSoucery ARM toolchain, instead of download just the 97 MB of the toolchain, you would download 97 MB + 278 MB of source code. You're basically quadrupling the data to be downloaded for the toolchain itself.

* Now, after a successful build, "make source" will no longer be a no-op operation, because "make source" will also download the files needed for legal-info. This is IMO rather unexpected.

Now whether those two concerns are really sufficient to justify a different implementation can be discussed. One possible solution is to have a "make legal-info-source", which would download whatever legal-info needs to be executed offline.

That being said, we're really talking about a few corner cases, because essentially, only the external toolchains currently make use of <pkg>_ACTUAL_SOURCE.

Thomas

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Thomas, All,

On 2016-03-19 15:47 +0100, Thomas Petazzoni spake thusly:

I think you (sort of) got it backwards.

The reason is not about doing "make source; make legal-info", but to be able to do "make source" and store all in a local mirror for example.

Companies like not to rely on third-party /storage/ and want to mirror the whole world, just in case (and that's sane; one can not rely on a third party for legal stuff, especially one that is not sub-contracted to that effect, like most FLOSS projects are).

So, really, the case for "make source" to also retrieve the actual sources is about being able to not rely on the upstream for legal-info.

Yes, this is unfortunate when one is not interested in legal compliance (e.g. for a purely internal use without redistribution).

I see. This can indeed be seen as unexpected.

However, I was pondering another change, whereby the build would *always* trigger legal-info. I am absolutely not sure about that one, but I like that "make" generates *all* output artifacts, of which I consider legal-info to be part. But this is for another day, if ever...

I'm not so sure. One interesting point in hooking into "make source" is that everyine already knows about it. I am not too fond of adding yet another top-level make target...

But if that would allow for the merging of that series faster, I'd be OK with that.

And when the user has a local download directory outside of Buildroot, it acts as a cache, and it will be doewnloaded only once.

Regards,
Yann E. MORIN.

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

Thomas, All,

On 2016-03-19 15:47 +0100, Thomas Petazzoni spake thusly:

One situation is a company that has a build farm wihch does not have acces to the internet, except for one machine that is only used to run "make source" to populate a mirror (either a "primary mirror" or an NFS-mounted directory) of the archives, that the other machines have acces to and can then tap into.

In this case, the machines running "make legal-info" do not have acces to the internet, so we need to have "make source" be complete, so it has to also download the actual sources. This is an off-line legal-info.

Regards,
Yann E. MORIN.

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

Introduce a new per-package variable to store the 'rawname-version' tuple, instead of computing it every time we need it.

Currently, it's only a single location, but follow-up patches will introduce more use of it.

---
 package/pkg-generic.mk | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index ce5525c..312290d 100644
@@ -393,6 +393,7 @@ else
  $(2)_DL_VERSION := $$(strip $$($(2)_VERSION))
 endif
 $(2)_VERSION := $$(call sanitize,$$($(2)_DL_VERSION))
+$(2)_RAWNAME_VERSION = $$($(2)_RAWNAME)-$$($(2)_VERSION)
 
 ifdef $(3)_OVERRIDE_SRCDIR
   $(2)_OVERRIDE_SRCDIR ?= $$($(3)_OVERRIDE_SRCDIR)

@@ -429,7 +430,7 @@ ifndef $(2)_SOURCE
  ifdef $(3)_SOURCE
   $(2)_SOURCE = $$($(3)_SOURCE)
  else
-  $(2)_SOURCE			?= $$($(2)_RAWNAME)-$$($(2)_VERSION).tar.gz
+  $(2)_SOURCE			?= $$($(2)_RAWNAME_VERSION).tar.gz
  endif
 endif
 

1.9.1

Dear Yann E. MORIN,

Since we have:

$(2)_BASE_NAME = $(1)-$$($(2)_VERSION)

I would prefer this new variable to be declared right after this one, and named $(2)_RAW_BASE_NAME or $(2)_RAWBASE_NAME.

Thanks

Thomas

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Thomas, All,

On 2016-03-19 15:48 +0100, Thomas Petazzoni spake thusly:

OK.

Regards,
Yann E. MORIN.

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

Currently, we put all source archives side-by-side in the same directory.

Since we're about to also save individual patches that were applied on those sources, we don't want to make that directory a complete mess of unassorted files.

So, we install each source archive in its own sub-directory, where we'll later store the patches too. Store that location in a variable, so it can be re-used later on (to install patches in a future commit).

---
Changes v1 -> v2:
- perl no longer has a post-legal-info hook (Thomas, Luca)

---
 package/pkg-generic.mk | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index 312290d..31f3e17 100644
@@ -496,6 +496,8 @@ endif
 
 $(2)_REDISTRIBUTE		?= YES
 
+$(2)_REDIST_SOURCES_DIR = $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4)))/$$($(2)_RAWNAME_VERSION)
+
 # When a target package is a toolchain dependency set this variable to
 # 'NO' so the 'toolchain' dependency is not added to prevent a circular
 # dependency

@@ -825,7 +827,7 @@ ifeq ($$($(2)_REDISTRIBUTE),YES)
 # Save the source tarball
 	$$(Q)$$(call hardlink-copy,\
 		     $$(DL_DIR)/$$($(2)_ACTUAL_SOURCE_TARBALL),\
-		     $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4))))
+		     $$($(2)_REDIST_SOURCES_DIR))
 endif # redistribute
 
 endif # other packages

1.9.1

Yann,

I was about to apply this one, but it depends on the hardlink-copy thingy, for which I had comments.

Thanks!

Thomas

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Now that we save the source archives in a directory named after the package and its version, do the same for the license files, for consistency.

It has a not-so-bad side-effect of also saving the version string in the all-licenses list.

The only (small) side-effect, is that the warnings about undefined _LICENSE_FILES now contains the version string, too. That's unavoidable, since that's what is stored in the legal report.

---
Changes v1 -> v2:
- s/drawback/side-effect/ (Luca)

---
 package/pkg-generic.mk | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index 31f3e17..4515b40 100644
@@ -807,10 +807,10 @@ ifneq ($$(call qstrip,$$($(2)_SOURCE)),)
 # is that the license still applies to the files distributed as part
 # of the rootfs, even if the sources are not themselves redistributed.
 ifeq ($$(call qstrip,$$($(2)_LICENSE_FILES)),)
-	@$$(call legal-license-nofiles,$$($(2)_RAWNAME),$$(call UPPERCASE,$(4)))
-	@$$(call legal-warning-pkg,$$($(2)_RAWNAME),cannot save license ($(2)_LICENSE_FILES not defined))
+	@$$(call legal-license-nofiles,$$($(2)_RAWNAME_VERSION),$$(call UPPERCASE,$(4)))
+	@$$(call legal-warning-pkg,$$($(2)_RAWNAME_VERSION),cannot save license ($(2)_LICENSE_FILES not defined))
 else
-	@$$(foreach F,$$($(2)_LICENSE_FILES),$$(call legal-license-file,$$($(2)_RAWNAME),$$(F),$$($(2)_DIR)/$$(F),$$(call UPPERCASE,$(4)))$$(sep))
+	@$$(foreach F,$$($(2)_LICENSE_FILES),$$(call legal-license-file,$$($(2)_RAWNAME_VERSION),$$(F),$$($(2)_DIR)/$$(F),$$(call UPPERCASE,$(4)))$$(sep))
 endif # license files
 
 ifeq ($$($(2)_SITE_METHOD),local)

1.9.1

Currently, we only store the filename of the applied patches.

However, we are soon to want to install those patches in the legal-info directory, so we'll have to know where those patches come from.

Instead of duplicating the logic to find the patches (bundled, downloaded, from a global patch dir...), just store the full path to each of those patches so we can retrieve them more easily later on.

Also always create the list-file, even if empty, so that we need not test for its existence before reading it.

---
Changes v1 -> v2:
- do not duplicate '/' in paths (Luca)

---
 support/scripts/apply-patches.sh | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

index 201278d..20a1552 100755
@@ -63,8 +63,12 @@ find ${builddir}/ '(' -name '*.rej' -o -name '.*.rej' ')' -print0 | \
     xargs -0 -r rm -f
 
 function apply_patch {
-    path=$1
-    patch=$2
+    path="${1%%/}"
+    patch="${2}"
+    case "${path}" in
+        /*) ;;
+        *)  path="$(pwd)/${path}";;
+    esac
     if [ "$3" ]; then
         type="series"; uncomp="cat"
     else

@@ -99,7 +103,7 @@ function apply_patch {
         echo "Error: missing patch file ${path}/$patch"
         exit 1
     fi
-    echo $patch >> ${builddir}/.applied_patches_list
+    echo "${path}/${patch}" >> ${builddir}/.applied_patches_list
     ${uncomp} "${path}/$patch" | patch -g0 -p1 -E -d "${builddir}" -t -N $silent
     if [ $? != 0 ] ; then
         echo "Patch failed!  Please fix ${patch}!"

@@ -141,6 +145,7 @@ function scan_patchdir {
     fi
 }
 
+touch ${builddir}/.applied_patches_list
 scan_patchdir "$patchdir" "$patchpattern"
 
 # Check for rejects...

1.9.1

Yann,

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Thomas, All,

On 2016-03-19 16:03 +0100, Thomas Petazzoni spake thusly:

Ah, indeed it's not trivial. When we apply our bundled patches, only the relative path to those patches are stored. So we make them canonical by prepending the current path to that relative path.

Strictly speaking, I am not sure it is needed, but for consistency, all paths are stored as full paths.

I know that feeling, that makes one not comfortable and bothered. Yet, I fail to see a problem in there; there are other cases where we save stuff in a file, and re-use it later from another place, like the graph-size stuff.

BTW, what did we so far save the list of patches for to begin with?

Hmmm... I consider all our support/scripts/ stuff to *be* part of the infra, like the mkusers script. apply-patches *is* part of the infra; it's just written in a shel script rather than in make (which would undoubtly be unreadable). Also, remember you suggested we offload all out complex make code for handling external toolchains, into a (set of) well scripts? It is similar to me.

What I don't like in this case, is that you decorelate applying the patches from listing them. By storing the list of patches at the time they are applied, we guarantee the list is consistent with the patches that were applied. By postponing the listing later, we can no longer offer that guarantee.

Yes, we can't guarantee that the patches we later copy are the ones that were actually applied; true. That's why I believe that legal-info should be part of the "standard" build.

Anyway, I won't fight that one; all that I care about is that patches are actually saved, so I'll go for the -l option to apply-patches.

Thanks!

Regards,
Yann E. MORIN.

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

Thomas, All,

On 2016-03-19 19:51 +0100, Yann E. MORIN spake thusly:

[--SNIP--]

e-reading this, I don;t see a proposal in there. Did I miss something, or did you forget to add something? ;-)

Well, it is in fact a bit more complex than just running apply-patches to get the list of patches.

First, some package do call apply-patches manually; there are 15 such packages, some cal.ling apply-patches more than once (gcc, linux, linux headers, uboot).

Second, in that case, some patches are applied conditionally. We do not want to duplicate that logic for legal-info.

So, I stand that the best solution is my proposal, to store the list of applied patches at the tiem they are applied, and use that later on for the legal-info output.

If there is a better proposal, or a simple way to fix yours, I'm all ears.

Thanks! :-)

Regards,
Yann E. MORIN.

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

Hello,

Well, the proposal is to make the infrastructure aware of patches that are applied.

As suggested above, one way of doing this is to extend the infrastructure so that no package does a manual APPLY_PATCHES call. I haven't looked at the 15 different places where we call APPLY_PATCHES manually, so I have no idea what is the complexity of handling that at the infrastructure level, but it looks like some extension to the infrastructure would allow to radically simplify the logic to apply patches in U-Boot, Barebox, Linux, AT91Bootstrap, etc. Indeed, the only reason why they have special logic is because the infrastructure doesn't allow you passing a local directory in <pkg>_PATCH.

Then, for packages like cvs, input-tools, mii-diag, netcat-openbsd, setserial, sysvinit and thttpd, the only reason why they call APPLY_PATCHES is to apply Debian patches, which are already part of the sources. So in fact, for those packages, your proposed logic would be somewhat incorrect, as it would store patches which are already in the source tarball.

To me, your proposal of having apply-patches.sh fill in a file goes completely backward compared to what we've done with _EXTRA_DOWNLOADS. We've added _EXTRA_DOWNLOADS precisely to make sure that the infrastructure is aware of every file we download, instead of having random packages do their own downloading.

Thomas

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

From a philosophical standpoint, I think this is indeed the best approach.
However, since even the simpler proposal below is already going to be very complicated, and since this series already consists of 16 patches, I would propose to leave that for later and accept Yann's approach for the time being. It's not so horrible to be unacceptable IMHO, it's just not great.

Regards,
Arnout

Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

Currently, the legal-info infra only saves the source archive of a package. However, that's not enough as we may apply some patches on packages sources.

We do suggest users to also redistribute the Buildroot sources as part of their compliance distribution, so the patches bundled in Buildroot would indeed be included in the compliance distribution.

However, that's still not enough, since we may download some patches, or the user may use a global patch directory. Patches in there might not end up in the compliance distribution, and there are risks of non-conformity.

So, always include patches alongside the source archive.

To ensure reproducibility, we also generate a series file, so patches can be re-applied in the correct order.

We get the list of patches to include from the list of patches that were applied by the package infrastructure (via the apply-patches support script). So, we need to get packages properly extracted and patched before we can save their legal-info, not just in the case they define _LICENSE_FILES.

Update the legal-info header accordingly.

Note: this means that, when a package is not patched and defines no LICENSE_FILES, we will extract and patch it for nothing. There is no easy way to know whether we have to patch a package or not. We can only either duplicate the logic to detect patches (bad) or rely on the infra actually patching the package. Also, a vast majority of packages are either patched, or define _LICENSE_FILES, so it is best and easiest to always extract and patch them prior to legal-info.

---
Changes v3 -> v4:
- typo (Luca)

Changes v2 -> v3:
- also mention that patches have been saved (Luca)

Changes v1 -> v2:
- don't recompute rawname-version needlessly (Luca)

---
 package/pkg-generic.mk           | 13 ++++++++-----
 support/legal-info/README.header |  9 +++++----
 2 files changed, 13 insertions(+), 9 deletions(-)

index 4515b40..3a6f62b 100644
@@ -778,12 +778,10 @@ $(2)_MANIFEST_LICENSE_FILES = $$($(2)_LICENSE_FILES)
 endif
 $(2)_MANIFEST_LICENSE_FILES ?= not saved
 
-# If the package declares _LICENSE_FILES, we need to extract it,
-# for overriden, local or normal remote packages alike, whether
-# we want to redistribute it or not.
-ifneq ($$($(2)_LICENSE_FILES),)
+# We need to extract and patch a package to be able to retrieve its
+# license files (if any) and the list of patches applied to it (if
+# any).
 $(1)-legal-info: $(1)-patch
-endif
 
 # We only save the sources of packages we want to redistribute, that are
 # non-overriden (local or true override).

@@ -828,6 +826,11 @@ ifeq ($$($(2)_REDISTRIBUTE),YES)
 	$$(Q)$$(call hardlink-copy,\
 		     $$(DL_DIR)/$$($(2)_ACTUAL_SOURCE_TARBALL),\
 		     $$($(2)_REDIST_SOURCES_DIR))
+# Save patches and generate the series file
+	$$(Q)while read f; do \
+		$$(call hardlink-copy,$$$${f},$$($(2)_REDIST_SOURCES_DIR)) || exit 1; \
+		printf "%s\n" "$$$${f##*/}" >>$$($(2)_REDIST_SOURCES_DIR)/series || exit 1; \
+	done <$$($(2)_DIR)/.applied_patches_list
 endif # redistribute
 
 endif # other packages

index d07c45d..418de14 100644
@@ -14,10 +14,11 @@ This material is composed of the following items.
    compiled programs.
    Note: this may have not been saved due to technical limitations, you may
    need to collect it manually.
- * The source code for all packages; this has been saved in the sources/
-   subdirectory (except for the non-redistributable packages, which have not
-   been saved); patches applied to some packages by Buildroot are included in
-   the Buildroot sources and were not duplicated in the sources/ subdirectory.
+ * The original source code for all packages; this has been saved in the
+   sources/ subdirectory (except for the non-redistributable packages, which
+   have not been saved). Patches that were applied are also saved, along
+   with a file named 'series' that lists the patches in the order they were
+   applied.
  * A manifest file listing the configured packages and related information.
  * The license text of the packages; they have been saved in the licenses/
    subdirectory.

1.9.1

Patches we save can come from various locations;
- bundled with Buildroot
- downloaded
- from one or more global-patch-dir

It is possible that two patches lying into different locations have the same basename, like so (first is bundled, second is from an hypothetical global-patch-dir):

    package/foo/0001-fix-Makefile.patch
    /path/to/my/patches/foo/0001-fix-Makefile.patch

In that case, we'd save only the second patch, overwriting the first.

We fix that by forcibly prefixing saved patches with a new numbering, to guarantee the unicity of saved files.

The unfortunate side-effects are that:
- most saved patches will be twice-numbered,
- the series file is now redundant.

While the former is mostly not nice-looking, we shouldn't try to strip any leading numbering, as that might not be a numbering (e.g. 42-retries.patch which is a patch add 42 retries).

The latter is not really problematic. A lot of tools (and people) can work with, and prefer to have a series file.

---
Note that I've made this a separate change, not because the patch would be too complex if squahed with the previous, but rather to have a more detailed commit log about the reason for the renumbering; squashing the two changes together would make for a really long commit log, at the risk of being more difficult to follow.

Note: an alternative solution was suggested by Luca (and Arnout?), which would be to check that no two patches have the same basename or bail-out otherwise. I choose to keep the renumbering, because it follows the path of least resistance (i.e. it does not break existing setups).

---
 package/pkg-generic.mk | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

index 3a6f62b..ed139fb 100644
@@ -827,9 +827,16 @@ ifeq ($$($(2)_REDISTRIBUTE),YES)
 		     $$(DL_DIR)/$$($(2)_ACTUAL_SOURCE_TARBALL),\
 		     $$($(2)_REDIST_SOURCES_DIR))
 # Save patches and generate the series file
-	$$(Q)while read f; do \
-		$$(call hardlink-copy,$$$${f},$$($(2)_REDIST_SOURCES_DIR)) || exit 1; \
-		printf "%s\n" "$$$${f##*/}" >>$$($(2)_REDIST_SOURCES_DIR)/series || exit 1; \
+# Because patches may come from various places (bundled in Buildroot,
+# from one or more global-patch-dir), there might be collisions on the
+# basenames of those files.
+# We add a new numbering to each patch to ensure unicity of the filenames.
+	$$(Q)patch_num=1; while read f; do \
+		prefix_num=$$$$(printf "%04d" $$$${patch_num}); \
+		$$(call hardlink-copy,$$$${f},$$($(2)_REDIST_SOURCES_DIR),\
+			$$$${prefix_num}-$$$${f##*/}) || exit 1; \
+		printf "%s\n" "$$$${prefix_num}-$$$${f##*/}" >>$$($(2)_REDIST_SOURCES_DIR)/series || exit 1; \
+		: $$$$((patch_num++)); \
 	done <$$($(2)_DIR)/.applied_patches_list
 endif # redistribute
 

1.9.1

Dear Yann E. MORIN,

I second the position of Luca and Arnout. This renumbering is complicated and leads to ugly patch names that no longer match the original file name. So please simply bail-out if two patches have the same name. It's highly unlikely, so I don't think we should renumber all the patches and have different file names just to avoid this unlikely case.

Thomas

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Thomas, All,

On 2016-03-19 16:05 +0100, Thomas Petazzoni spake thusly:

[--SNIP--]

Done.

Thanks! :-)

Regards,
Yann E. MORIN.

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

Some packages, like perl, download extra files that end up as part of the source that Buildroot builds. Up until now, those files were not saved in the legal-info output.

Add those files to the legal-info output.

The unfortunate side-effect is that we will also save the secondary archive for the external blackfin toolchains; however, we already do save the binary release of some external toolchains when they do not provide actual source archives.

This is inherently bad, as those are not source archives, but solving this is a bigger concern, for another series...

---
Changes v2 -> v3:
- typo (Luca)
- incorporate the post-commit log message (the part about the side-effect) into the commit log itself, it makes sense to not forget about that

---
 package/pkg-generic.mk | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

index ed139fb..bc39ead 100644
@@ -822,10 +822,12 @@ else
 # Other packages
 
 ifeq ($$($(2)_REDISTRIBUTE),YES)
-# Save the source tarball
-	$$(Q)$$(call hardlink-copy,\
-		     $$(DL_DIR)/$$($(2)_ACTUAL_SOURCE_TARBALL),\
-		     $$($(2)_REDIST_SOURCES_DIR))
+# Save the source tarball and any extra downloads, but not
+# patches, as they are handled specially afterwards.
+	$$(foreach e,$$($(2)_ACTUAL_SOURCE_TARBALL) $$(notdir $$($(2)_EXTRA_DOWNLOADS)),\
+			$$(Q)$$(call hardlink-copy,\
+				$$(DL_DIR)/$$(e),\
+				$$($(2)_REDIST_SOURCES_DIR))$$(sep))
 # Save patches and generate the series file
 # Because patches may come from various places (bundled in Buildroot,
 # from one or more global-patch-dir), there might be collisions on the

1.9.1

Dear Yann E. MORIN,

I see two possible ways around this:

- Have two separate variables, like <pkg>_EXTRA_SOURCE (to be used for perl, contains real source code that needs to be saved during legal-info), and <pkg>_EXTRA_DOWNLOADS (to be used for external toolchains).

- Just like you don't copy <pkg>_SOURCE to legal-info when <pkg>_ACTUAL_SOURCE is defined, don't copy <pkg>_EXTRA_DOWNLOADS when <pkg>_ACTUAL_SOURCE is defined.

Thomas

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

I don't like this very much, because it adds infra for something that is used
only once.

This sounds like a good idea. However, I'm sure that at some point we'll run
into a conflict (i.e. a package that has ACTUAL_SOURCE defined but also wants EXTRA DOWNLOADS of sources). To be solved when that happens, but we need a good commit message or comment that basically summarises this entire discussion so we know the implications when that happens.

Regards,
Arnout

Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

Having a hash of the saved files can be interesting for the recipient to verify the integrity of the files.

We remove the warning file earlier, to exclude it from the hash list.

---
Changes v1 -> v2:
- simplify getting rid of the ..../legal-info/ prefix (Luca)
- always sort with the C locale

---
 Makefile | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

index 98c8dc7..4c13a9d 100644
@@ -696,8 +696,14 @@ legal-info: dirs legal-info-clean legal-info-prepare $(foreach p,$(PACKAGES),$(p
 		cat support/legal-info/README.warnings-header \
 			$(LEGAL_WARNINGS) >>$(LEGAL_REPORT); \
 		cat $(LEGAL_WARNINGS); fi
-	@echo "Legal info produced in $(LEGAL_INFO_DIR)"
 	@rm -f $(LEGAL_WARNINGS)
+	@(cd $(LEGAL_INFO_DIR); \
+	  find * -type f -exec sha256sum {} + \
+	  |LC_ALL=C sort -k2 \
+	  >.legal-info.sha256; \
+	  mv .legal-info.sha256 legal-info.sha256 \
+	)
+	@echo "Legal info produced in $(LEGAL_INFO_DIR)"
 
 show-targets:
 	@echo $(PACKAGES) $(TARGETS_ROOTFS)

1.9.1

Yann,

It's adding more stuff to the main Makefile (while I'd like to remove stuff from that Makefile), for something that is quite specific. But OK, I can see the usefulness of this hash file, so:

However, I'm really surprised by this LEGAL_WARNINGS file. It's exactly the type of hidden file (like .applied_patches) I don't like. One example where it fails badly is if you interrupt "legal-info" in the middle. Since the legal-warning and legal-warning-pkg macro only concatenate to this file, then at the end of the second, non-interrupted, make invocation, you would got some warnings from the first run.

Why aren't we simply displaying the warnings as we go? Probably because they were mixed with some other messages (like the messages about download some stuff). But in this case, we can serialize the steps by having all the download done first, then all the "aggregation" done, so that warnings can be displayed directly without this hidden file.

Of course, this is unrelated to your change, and more a comment for Luca.

Thomas

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Thomas, All,

On 2016-03-19 16:21 +0100, Thomas Petazzoni spake thusly:

Nope, because the very first thing we do for legal-info is to trash the existing directory before starting to collect any legal-info.

However, I would contend that we should leave that file in place. It is critical that some stuff could not be saved, so that all-caps file is a godd indication that something is wrong.

Yes, we could do that, but I'd still keep the file. Delegate to the user the responsibility to remove it.

I can have a look as a follow-up series.

Regards,
Yann E. MORIN.

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

It might be necessary to not even mention a package in the output of legal-info:

- virtual packages have virtually nothing to save in the legal-info

    output;

- for Buildroot itself, host-gcc-initial and host-gcc-final are not real packages, they are just two different steps of the same package, gcc;

- for proprietary packages, it might not even be legal to even mention them, being under NDA or some other such restrictive conditions.

Introduce the new FOO_LEGAL_INGORE variable that a package can set to 'YES' (default to 'NO') to indicate that the package should be completely ignored from the legal-info output, in which case the package is not mentioned in the maniufest, its source archive, patches and license files are not saved into legal-info/ .

---
Changes v1 -> v2:
- introduce a new variable, instead of making _REDISTRIBUTE a tri-state (Thomas, Peter, Luca)

---
 docs/manual/adding-packages-generic.txt |  9 +++++++++
 package/pkg-generic.mk                  | 15 +++++++++++++++
 2 files changed, 24 insertions(+)

index 8ed7fe8..5cf2ae0 100644
@@ -430,6 +430,15 @@ information is (assuming the package name is +libfoo+) :
   non-opensource packages: Buildroot will not save the source code for this
   package when collecting the +legal-info+.
 
+* +LIBFOO_LEGAL_IGNORE+ can be set to +YES+ or +NO+ (the default) to indicate
+  that this package should be completely ignored when saving the licensing
+  information. If set to +YES+, then the package is not listed in the manifest,
+  its source archive and its license files are not saved. You probably do not
+  want to set it to +YES+, unless under very specific conditions (e.g. when
+  you use the `legal-info/` output directory as-is to provide a compliance
+  delivery, and do not want your proprietary, non-redistributable packages to
+  even be mentioned in the manifest).
+
 * +LIBFOO_FLAT_STACKSIZE+ defines the stack size of an application built into
   the FLAT binary format. The application stack size on the NOMMU architecture
   processors can't be enlarged at run time. The default stack size for the

index bc39ead..3a5ad83 100644
@@ -496,6 +496,14 @@ endif
 
 $(2)_REDISTRIBUTE		?= YES
 
+ifndef $(2)_LEGAL_IGNORE
+ ifdef $(3)_LEGAL_IGNORE
+  $(2)_LEGAL_IGNORE = $$($(3)_LEGAL_IGNORE)
+ endif
+endif
+
+$(2)_LEGAL_IGNORE		?= NO
+
 $(2)_REDIST_SOURCES_DIR = $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4)))/$$($(2)_RAWNAME_VERSION)
 
 # When a target package is a toolchain dependency set this variable to

@@ -781,7 +789,10 @@ $(2)_MANIFEST_LICENSE_FILES ?= not saved
 # We need to extract and patch a package to be able to retrieve its
 # license files (if any) and the list of patches applied to it (if
 # any).
+# But not if we want to ignore that package completely.
+ifneq ($$($(2)_LEGAL_IGNORE),YES)
 $(1)-legal-info: $(1)-patch
+endif
 
 # We only save the sources of packages we want to redistribute, that are
 # non-overriden (local or true override).

@@ -794,6 +805,8 @@ endif
 
 # legal-info: produce legally relevant info.
 $(1)-legal-info:
+ifneq ($$($(2)_LEGAL_IGNORE),YES)
+
 # Packages without a source are assumed to be part of Buildroot, skip them.
 	$$(foreach hook,$$($(2)_PRE_LEGAL_INFO_HOOKS),$$(call $$(hook))$$(sep))
 ifneq ($$(call qstrip,$$($(2)_SOURCE)),)

@@ -847,6 +860,8 @@ endif # other packages
 endif # ifneq ($$(call qstrip,$$($(2)_SOURCE)),)
 	$$(foreach hook,$$($(2)_POST_LEGAL_INFO_HOOKS),$$(call $$(hook))$$(sep))
 
+endif # $(2)_LEGAL_IGNORE != YES
+
 # add package to the general list of targets if requested by the buildroot
 # configuration
 ifeq ($$($$($(2)_KCONFIG_VAR)),y)

1.9.1

Dear Yann E. MORIN,

What is the difference with <pkg>_REDISTRIBUTE = NO ? I know REDISTRIBUTE = NO packages are mentioned in legal-info, but their source code is not copied to the legal-info stuff.

But does it make sense to have two separate things? Why do REDISTRIBUTE = NO packages get mentioned in the legal-info if their source code is anyway not saved.

typoe: IGNORE

typo: manifest

Ah, we discussed using REDISTRIBUTE, I remember. But do we need a tri-state ? Do we really have REDISTRIBUTE = NO packages that we want to see mentioned in the legal-info output ?

To me, the naming of the variable looks like inverted logic. What about:

LIBFOO_SAVE_LEGAL_INFO = YES (default) / NO

but obviously, <pkg>_SAVE_LEGAL_INFO is a bit confusing with <pkg>_REDISTRIBUTE.

Best regards,

Thomas

Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Thomas, All,

On 2016-03-19 16:29 +0100, Thomas Petazzoni spake thusly:

"FOO_LEGAL_IGNORE = YES" mnens that the package will not even be listed in the manifest.

Sometimes, there are (proprietary) packages that are under NDA, and even the mere hint at the use of that package is forbidden. So we need to be able to represent that situation.

Because the license may require it? For example, the boot codes for the RPi are BSD-licensed, so we have to provide the license file, so rpi-firmware has to be in the manifest. But we do not have the source for those blobs...

[--SNIP--]
Typoes fixed, thanks.

Yes, see above.

Yes, I am not completely sold on the _LEGAL_IGNORE name, but your proposal is not better IMHO...

Regards,
Yann E. MORIN.

.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

Virtual packages really have nothing to save for legal-info, so we just ignore them, to avoid spurious "extracting" and "patching" messages while running legal-info.

---
Changes v3 -> v4:
- rephrase the .Note (Luca)
- drop now-incorrect blurb from the commit log (Luca)

Changes v1 -> v2:
- use the new _LEGAL_IGNORE variable (Thomas, Luca, Peter)

---
 docs/manual/adding-packages-virtual.txt | 5 +++++
 package/pkg-virtual.mk                  | 2 ++
 2 files changed, 7 insertions(+)

index a5f17a2..3857927 100644
@@ -56,6 +56,11 @@ The +.mk+ for the virtual package should just evaluate the +virtual-package+ mac
 The ability to have target and host packages is also available, with the
 +host-virtual-package+ macro.
 
+.Note:
+The +virtual-package+ infrastructure automatically marks virtual packages to be
+excluded from the +legal-info+ output (by internally setting +FOO_LEGAL_IGNORE+
+to 'YES').
+
 ==== Provider's +Config.in+ file
 
 When adding a package as a provider, only the +Config.in+ file requires some

index 244c1d0..445f09a 100644
@@ -59,6 +59,8 @@ endif
 # Add dependency against the provider
 $(2)_DEPENDENCIES += $$(call qstrip,$$(BR2_PACKAGE_PROVIDES_$(2)))
 
+$(2)_LEGAL_IGNORE = YES
+
 # Call the generic package infrastructure to generate the necessary
 # make targets
 $(call inner-generic-package,$(1),$(2),$(3),$(4))

1.9.1

Regards,
Arnout

Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

From: Luca Ceresoli <[email protected]>

---
 support/legal-info/README.header | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 418de14..feee104 100644
@@ -18,7 +18,7 @@ This material is composed of the following items.
    sources/ subdirectory (except for the non-redistributable packages, which
    have not been saved). Patches that were applied are also saved, along
    with a file named 'series' that lists the patches in the order they were
-   applied.
+   applied. Patches are under the same license as the original package.
  * A manifest file listing the configured packages and related information.
  * The license text of the packages; they have been saved in the licenses/
    subdirectory.

1.9.1