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

When preparing the legal-info, the source archives are copied in the legal-info/ output directory. When the archives are big, it can take quite a bit of time and unnecessarily uses disk space. When the legal-info output directory is on the same filesystem as the BR2_DL_DIR, we can easily reduce copy time and disk usage by just using hardlins instead of copying. However, the BR2_DL_DIR may be on a different filesystem, so we must fallback to copying in this case

Introduce a helper script that copies a source file into a destination directory, by first attempting to hard-link, and falling back to a plain copy in case the hardlink fails.

In case the destination already exists, it is forcibly removed first, to avoid clobering any existing target file (and especially any hardlink to it), since cp -f does not remove the destination file, but clobbers it.

In some situations, it will be necessary that the destination file is named differently than the source, so if a third argument is specified, it is treated as the basename of the destination file.

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Luca Ceresoli <[email protected]>

When preparing the legal-info, the source archives are copied in the legal-info/ output directory. When the archives are big, it can take quite a bit of time and unnecessarily uses disk space. When the legal-info output directory is on the same filesystem as the BR2_DL_DIR, we can easily reduce copy time and disk usage by just using hardlins instead of copying. However, the BR2_DL_DIR may be on a different filesystem, so we must fallback to copying in this case

Introduce a helper script that copies a source file into a destination directory, by first attempting to hard-link, and falling back to a plain copy in case the hardlink fails.

In case the destination already exists, it is forcibly removed first, to avoid clobering any existing target file (and especially any hardlink to it), since cp -f does not remove the destination file, but clobbers it.

In some situations, it will be necessary that the destination file is named differently than the source, so if a third argument is specified, it is treated as the basename of the destination file.

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Luca Ceresoli <[email protected]>

---
Changes v6 -> v7:
- more explicit variables (Arnout)
- drop the variable with the helper path (Arnout)
- use shell substitution instead of $(basename ...) (Arnout! ;- )
- improve commit log to explain the 'rm -f'
- split now-too-long line

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

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Luca Ceresoli <[email protected]>

---
Changes v6 -> v7:
- directly call the wrapper without the variable pointing to it (Arnout)
- fix left-over comma (Arnout)

Changes v5 -> v6:
- directly call the helper script (Thomas, Arnout)
- drop Luca's and Arnout's tags, as the code did change a bit

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(-)

package/pkg-generic.mk +4 -3

index 3904c09..f9c7cf8 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)support/scripts/hardlink-or-copy \
+		$$(DL_DIR)/$$($(2)_ACTUAL_SOURCE_TARBALL) \
+		$$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4)))
 endif # redistribute
 
 endif # other packages

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

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Luca Ceresoli <[email protected]>

---
Changes v6 -> v7:
- directly call the wrapper without the variable pointing to it (Arnout)
- fix left-over comma (Arnout)

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.

Reported-by Luca Ceresoli <[email protected]>

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Luca Ceresoli <[email protected]>

Reviewed-by Luca Ceresoli <[email protected]>

Reviewed-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

Cc Thomas Petazzoni <[email protected]>

---
Changes v5 -> v6:
- rename variable (Thomas)

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

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).

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Luca Ceresoli <[email protected]>

Acked-by Luca Ceresoli <[email protected]>

Tested-by Luca Ceresoli <[email protected]>

Reviewed-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

Reviewed-by Thomas Petazzoni <[email protected]>

---
Changes v6 -> v7:
- directly call the helper

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.

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Luca Ceresoli <[email protected]>

Acked-by Luca Ceresoli <[email protected]>

Tested-by Luca Ceresoli <[email protected]>

---
Chages v5 -> v6:
- raw name variable was renamed (Thomas)

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

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

package/pkg-generic.mk +0 -0

index b031879..7a86355 100644

package/pkg-generic.mk +1 -1

 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)_RAW_BASE_NAME),$$(F),$$($(2)_DIR)/$$(F),$$(call UPPERCASE,$(4)))$$(sep))
 endif # license files
 
 ifeq ($$($(2)_SITE_METHOD),local)

package/pkg-generic.mk ifneq ($$(call qstrip,$$($(2)_SOURCE)),) · line 790

Quoted 1 line

package/pkg-generic.mk +1 -0

+	@$$(call legal-warning-pkg,$$($(2)_RAW_BASE_NAME),cannot save license ($(2)_LICENSE_FILES not defined))  

I think this should stay $(2)_RAWNAME, or either it should change as well for
the legal-warning-source below.

Other than that:

Reviewed-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

Quoted 6 lines

package/pkg-generic.mk ifneq ($$(call qstrip,$$($(2)_SOURCE)),) · line 790

Quoted 2 lines

package/pkg-generic.mk +1 -0

+	@$$(call legal-warning-pkg,$$($(2)_RAW_BASE_NAME),cannot save license ($(2)_LICENSE_FILES not defined))  

I think this should stay $(2)_RAWNAME, or either it should change as well for
the legal-warning-source below.

Quoted 2 lines

Other than that:

Reviewed-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

I agree with Arnout here (we should keep using $(2)_RAWNAME for the warning). Yann, what do you think?

Thomas, All,

On 2016-05-11 23:43 +0200, Thomas Petazzoni spake thusly:

Quoted 7 lines · Arnout Vandecappelle

Quoted 6 lines

package/pkg-generic.mk ifneq ($$(call qstrip,$$($(2)_SOURCE)),) · line 790

Quoted 2 lines

package/pkg-generic.mk +1 -0

+	@$$(call legal-warning-pkg,$$($(2)_RAW_BASE_NAME),cannot save license ($(2)_LICENSE_FILES not defined))

I think this should stay $(2)_RAWNAME, or either it should change as well for
the legal-warning-source below.

Quoted 5 lines

Quoted 2 lines

Other than that:

Reviewed-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

I agree with Arnout here (we should keep using $(2)_RAWNAME for the warning). Yann, what do you think?

1. With this patch (and the following one), legal-info now gets saved in sub-directories named after the paclage name and version, i.e.:

legal-info/licenses/pkg-version/
legal-info/sources/pkg-version/

2. With this patch, we also store the package name and version in the manifest.

3. The warnings are stored in the legal-info report.

So, I think it is better that the info about packages is the same
everywhere: sources/ and licenses/ sub-dirs, entries in the manifest and warnings alike.

Quoted 2 lines

Quoted 2 lines

[...] or either it should change as well for
the legal-warning-source below.

Quoted 27 lines

Thomas, All,

On 2016-05-11 23:43 +0200, Thomas Petazzoni spake thusly:

Quoted 7 lines · Arnout Vandecappelle

Quoted 6 lines

package/pkg-generic.mk ifneq ($$(call qstrip,$$($(2)_SOURCE)),) · line 790

Quoted 2 lines

package/pkg-generic.mk +1 -0

+	@$$(call legal-warning-pkg,$$($(2)_RAW_BASE_NAME),cannot save license ($(2)_LICENSE_FILES not defined))

I think this should stay $(2)_RAWNAME, or either it should change as well for
the legal-warning-source below.

Quoted 5 lines

Quoted 2 lines

Other than that:

Reviewed-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

I agree with Arnout here (we should keep using $(2)_RAWNAME for the warning). Yann, what do you think?

1. With this patch (and the following one), legal-info now gets saved in sub-directories named after the paclage name and version, i.e.:

legal-info/licenses/pkg-version/
legal-info/sources/pkg-version/

2. With this patch, we also store the package name and version in the manifest.

3. The warnings are stored in the legal-info report.

Quoted 3 lines

So, I think it is better that the info about packages is the same
everywhere: sources/ and licenses/ sub-dirs, entries in the manifest and warnings alike.

Quoted 2 lines · I completely agree with this reasoning. Note that I

Quoted 2 lines

Quoted 2 lines

[...] or either it should change as well for
the legal-warning-source below.

Quoted 27 lines

Thomas, All,

On 2016-05-11 23:43 +0200, Thomas Petazzoni spake thusly:

Quoted 7 lines · Arnout Vandecappelle

Quoted 6 lines

package/pkg-generic.mk ifneq ($$(call qstrip,$$($(2)_SOURCE)),) · line 790

Quoted 2 lines

package/pkg-generic.mk +1 -0

+	@$$(call legal-warning-pkg,$$($(2)_RAW_BASE_NAME),cannot save license ($(2)_LICENSE_FILES not defined))

I think this should stay $(2)_RAWNAME, or either it should change as well for
the legal-warning-source below.

Quoted 5 lines

Quoted 2 lines

Other than that:

Reviewed-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

I agree with Arnout here (we should keep using $(2)_RAWNAME for the warning). Yann, what do you think?

1. With this patch (and the following one), legal-info now gets saved in sub-directories named after the paclage name and version, i.e.:

legal-info/licenses/pkg-version/
legal-info/sources/pkg-version/

2. With this patch, we also store the package name and version in the manifest.

3. The warnings are stored in the legal-info report.

Quoted 3 lines

So, I think it is better that the info about packages is the same
everywhere: sources/ and licenses/ sub-dirs, entries in the manifest and warnings alike.

Quoted 2 lines · I completely agree with this reasoning. Note that I

Quoted 2 lines

Quoted 2 lines

[...] or either it should change as well for
the legal-warning-source below.

Arnout, All,

On 2016-05-14 23:22 +0200, Arnout Vandecappelle spake thusly:

Quoted 28 lines · Yann E. MORIN

Quoted 27 lines

Thomas, All,

On 2016-05-11 23:43 +0200, Thomas Petazzoni spake thusly:

Quoted 7 lines · Arnout Vandecappelle

Quoted 6 lines

package/pkg-generic.mk ifneq ($$(call qstrip,$$($(2)_SOURCE)),) · line 790

Quoted 2 lines

package/pkg-generic.mk +1 -0

+	@$$(call legal-warning-pkg,$$($(2)_RAW_BASE_NAME),cannot save license ($(2)_LICENSE_FILES not defined))

I think this should stay $(2)_RAWNAME, or either it should change as well for
the legal-warning-source below.

Quoted 5 lines

Quoted 2 lines

Other than that:

Reviewed-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

I agree with Arnout here (we should keep using $(2)_RAWNAME for the warning). Yann, what do you think?

1. With this patch (and the following one), legal-info now gets saved in sub-directories named after the paclage name and version, i.e.:

legal-info/licenses/pkg-version/
legal-info/sources/pkg-version/

2. With this patch, we also store the package name and version in the manifest.

3. The warnings are stored in the legal-info report.

Quoted 8 lines

Quoted 3 lines

So, I think it is better that the info about packages is the same
everywhere: sources/ and licenses/ sub-dirs, entries in the manifest and warnings alike.

Quoted 2 lines · I completely agree with this reasoning. Note that I

Quoted 2 lines

Quoted 2 lines

[...] or either it should change as well for
the legal-warning-source below.

OK, but now that I look at it, and after Thomas pointed it on IRC, we can't really add the version for legal-warning-nosource (legal-warning-source does not exist, so I gues you meant legal-warning-nosource), because for the two occurences of those two, we don't have a version to beging with:

- one if about 'local' pacakges for which we do not have a version

    string at all;

- the second if for overriden packages, for which we do not have a version string either (well, the one in our .mk is probably wrong, so we should not use it).

So, do you still want to use the _RAW_BASE_NAME for those, even though it does not make sense (both would be rendered into 'foo-custom')?

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.

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Luca Ceresoli <[email protected]>

Acked-by Luca Ceresoli <[email protected]>

Tested-by Luca Ceresoli <[email protected]>

---
Chages v5 -> v6:
- raw name variable was renamed (Thomas)

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.

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Luca Ceresoli <[email protected]>

Reviewed-by Luca Ceresoli <[email protected]>

[Tested only with patches in the Buildroot sources]

Tested-by Luca Ceresoli <[email protected]>

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

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

support/scripts/apply-patches.sh +0 -0

index 201278d..20a1552 100755

support/scripts/apply-patches.sh +3 -1

+    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...

Quoted 9 lines

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.

Quoted 8 lines

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

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Luca Ceresoli <[email protected]>

Reviewed-by Luca Ceresoli <[email protected]>

[Tested only with patches in the Buildroot sources]

Tested-by Luca Ceresoli <[email protected]>

Reviewed-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

Small question...

Quoted 15 lines

---
Changes v1 -> v2:
- do not duplicate '/' in paths (Luca)
---
support/scripts/apply-patches.sh | 11 ++++++++---

1 file changed, 8 insertions(+), 3 deletions(-)

support/scripts/apply-patches.sh +0 -0

index 201278d..20a1552 100755
@@ -63,8 +63,12 @@ find ${builddir}/ '(' -name '*.rej' -o -name '.*.rej' ')' -print0 | \
    xargs -0 -r rm -f
 

support/scripts/apply-patches.sh find ${builddir}/ '(' -name '*.rej' -o -name '.*.rej' ')' -print0 | \ · line 70

Quoted 8 lines

support/scripts/apply-patches.sh +5 -2

 function apply_patch {
-    path=$1
-    patch=$2
+    path="${1%%/}"
+    patch="${2}"
+    case "${path}" in
+        /*) ;;
+        *)  path="$(pwd)/${path}";;

Is there a particular reason to prefer $(pwd) over $PWD?

Regards,
Arnout

Quoted 22 lines

support/scripts/apply-patches.sh +3 -1

+    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...

Signature · 6 lines

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, 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.

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Luca Ceresoli <[email protected]>

Reviewed-by Luca Ceresoli <[email protected]>

[Tested only with patches in the Buildroot sources]

Tested-by Luca Ceresoli <[email protected]>

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

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

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, when running legal-info, we'd save only the second patch, overwriting the first. That would be oproblematic, because:

- either the second patch depends on the first, and thus would no longer apply (this is easy to detect, though),

- or the second patch does not dpend on the first, and the compliance delivery will not be complete (this is much arder to detect).

We fix that by checking that no two patches have the same same basename. If we find that the basename of the patch to be applied collides with that of a previously applied patch, we error out and report the duplicate.

The unfortunate side-effect is that existing setups will now break in that situation, but that's a minor, corner-case issue that is easily fixed.

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Luca Ceresoli <[email protected]>

Cc Arnout Vandecappelle <[email protected]>

Cc Thomas Petazzoni <[email protected]>

---
Changes v6 -> v7:
- reword error message

Changes v5 -> v6:
- don't renumber, detect collision and error out (Luca, Arnout, Thomas)

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

support/scripts/apply-patches.sh +8 -0

index 20a1552..f8b6ca3 100755
@@ -103,6 +103,14 @@ function apply_patch {
         echo "Error: missing patch file ${path}/$patch"
         exit 1
     fi
+    existing="$( grep -E "/${patch}\$" ${builddir}/.applied_patches_list )"
+    if [ -n "${existing}" ]; then
+        echo "Error: duplicate filename '${patch}'"
+        echo "Conflicting files are:"
+        echo "  already applied: ${existing}"
+        echo "  to be applied  : ${path}/${patch}"
+        exit 1
+    fi
     echo "${path}/${patch}" >> ${builddir}/.applied_patches_list
     ${uncomp} "${path}/$patch" | patch -g0 -p1 -E -d "${builddir}" -t -N $silent
     if [ $? != 0 ] ; then

Quoted 24 lines

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, when running legal-info, we'd save only the second patch, overwriting the first. That would be oproblematic, because:

- either the second patch depends on the first, and thus would no longer apply (this is easy to detect, though),

- or the second patch does not dpend on the first, and the compliance delivery will not be complete (this is much arder to detect).

We fix that by checking that no two patches have the same same basename. If we find that the basename of the patch to be applied collides with that of a previously applied patch, we error out and report the duplicate.

Quoted 8 lines

The unfortunate side-effect is that existing setups will now break in that situation, but that's a minor, corner-case issue that is easily fixed.

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Luca Ceresoli <[email protected]>

Cc Arnout Vandecappelle <[email protected]>

Cc Thomas Petazzoni <[email protected]>

Reviewed-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

Regards,
Arnout

PS When I wrote earlier that this patch was "controversial", I actually
meant "a patch that is not strictly needed to fix the stuff-missing-from-legal-info issue, and which could be skipped".

Quoted 31 lines

---
Changes v6 -> v7:
- reword error message

Changes v5 -> v6:
- don't renumber, detect collision and error out (Luca, Arnout, Thomas)
---
support/scripts/apply-patches.sh | 8 ++++++++

1 file changed, 8 insertions(+)

support/scripts/apply-patches.sh +8 -0

index 20a1552..f8b6ca3 100755
@@ -103,6 +103,14 @@ function apply_patch {
        echo "Error: missing patch file ${path}/$patch"
        exit 1
    fi
+    existing="$( grep -E "/${patch}\$" ${builddir}/.applied_patches_list )"
+    if [ -n "${existing}" ]; then
+        echo "Error: duplicate filename '${patch}'"
+        echo "Conflicting files are:"
+        echo "  already applied: ${existing}"
+        echo "  to be applied  : ${path}/${patch}"
+        exit 1
+    fi
    echo "${path}/${patch}" >> ${builddir}/.applied_patches_list
    ${uncomp} "${path}/$patch" | patch -g0 -p1 -E -d "${builddir}" -t -N $silent
    if [ $? != 0 ] ; then

Signature · 6 lines

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

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, when running legal-info, we'd save only the second patch, overwriting the first. That would be oproblematic, because:

- either the second patch depends on the first, and thus would no longer apply (this is easy to detect, though),

- or the second patch does not dpend on the first, and the compliance delivery will not be complete (this is much arder to detect).

We fix that by checking that no two patches have the same same basename.

If we find that the basename of the patch to be applied collides with that of a previously applied patch, we error out and report the duplicate.

The unfortunate side-effect is that existing setups will now break in that situation, but that's a minor, corner-case issue that is easily fixed.

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Luca Ceresoli <[email protected]>

Cc Arnout Vandecappelle <[email protected]>

Cc Thomas Petazzoni <[email protected]>

---
Changes v6 -> v7:
- reword error message

Changes v5 -> v6:
- don't renumber, detect collision and error out (Luca, Arnout, Thomas)

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

support/scripts/apply-patches.sh +0 -0

index 20a1552..f8b6ca3 100755

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.

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Luca Ceresoli <[email protected]>

Tested-by Luca Ceresoli <[email protected]>

Reviewed-by Luca Ceresoli <[email protected]>

---
Changes v6 -> v7:
- directly call the helper

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           | 15 ++++++++++-----
 support/legal-info/README.header |  9 +++++----
 2 files changed, 15 insertions(+), 9 deletions(-)

package/pkg-generic.mk +3 -5

index 7a86355..13455d1 100644
@@ -749,12 +749,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).

package/pkg-generic.mk +2 -0

+		printf "%s\n" "$$$${f##*/}" >>$$($(2)_REDIST_SOURCES_DIR)/series || exit 1; \
+	done <$$($(2)_DIR)/.applied_patches_list
 endif # redistribute
 
 endif # other packages

support/legal-info/README.header +5 -4

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.

Quoted 34 lines

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.

Quoted 4 lines

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Luca Ceresoli <[email protected]>

Tested-by Luca Ceresoli <[email protected]>

Reviewed-by Luca Ceresoli <[email protected]>

Reviewed-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

But...

Quoted 36 lines

---
Changes v6 -> v7:
- directly call the helper

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 | 15 ++++++++++-----
support/legal-info/README.header | 9 +++++----

2 files changed, 15 insertions(+), 9 deletions(-)

package/pkg-generic.mk +0 -0

index 7a86355..13455d1 100644
@@ -749,12 +749,10 @@ $(2)_MANIFEST_LICENSE_FILES = $$($(2)_LICENSE_FILES)

endif
$(2)_MANIFEST_LICENSE_FILES ?= not saved

package/pkg-generic.mk +3 -4

-# 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

package/pkg-generic.mk endif · line 815

Quoted 11 lines

# We only save the sources of packages we want to redistribute, that are

# non-overriden (local or true override).

package/pkg-generic.mk +5 -0

@@ -810,6 +808,13 @@ endif
 	$$(Q)support/scripts/hardlink-or-copy \
 		$$(DL_DIR)/$$($(2)_ACTUAL_SOURCE_TARBALL) \
 		$$($(2)_REDIST_SOURCES_DIR)
+# Save patches and generate the series file
+	$$(Q)while read f; do \
+		support/scripts/hardlink-or-copy \
+			$$$${f} \
+			$$($(2)_REDIST_SOURCES_DIR) || exit 1; \

Instead of explicit exits, a set -e in the beginning would have been more compact.

I hate the $$$$, but not much can be done about it. Move the whole legal
info thing to a script, but that would require exporting a sh*tload of variables to that script...

Regards,
Arnout

Quoted 26 lines

package/pkg-generic.mk +2 -0

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

endif # redistribute

endif # other packages

support/legal-info/README.header +5 -4

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.

Signature · 6 lines

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.

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Luca Ceresoli <[email protected]>

Tested-by Luca Ceresoli <[email protected]>

Reviewed-by Luca Ceresoli <[email protected]>

---
Changes v6 -> v7:
- directly call the helper

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...

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Luca Ceresoli <[email protected]>

Cc Thomas Petazzoni <[email protected]>

Reviewed-by Luca Ceresoli <[email protected]>

Tested-by Luca Ceresoli <[email protected]>

---
Changes v6 -> v7:
- directly call the wrapper
- fix after re-ordering patches

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(-)

package/pkg-generic.mk +6 -4

index 13455d1..a720930 100644
@@ -804,10 +804,12 @@ 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)support/scripts/hardlink-or-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)support/scripts/hardlink-or-copy \
+			$$(DL_DIR)/$$(e) \
+			$$($(2)_REDIST_SOURCES_DIR)$$(sep))
 # Save patches and generate the series file
 	$$(Q)while read f; do \
 		support/scripts/hardlink-or-copy \

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...

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Luca Ceresoli <[email protected]>

Cc Thomas Petazzoni <[email protected]>

Reviewed-by Luca Ceresoli <[email protected]>

Tested-by Luca Ceresoli <[email protected]>

---
Changes v6 -> v7:
- directly call the wrapper
- fix after re-ordering patches

From: Luca Ceresoli <[email protected]>

Signed-off-by Luca Ceresoli <[email protected]>

Acked-by Thomas Petazzoni <[email protected]>

[yann.morin.1998 at free.fr: slightly tweak after Arnout's review]

Signed-off-by Yann E. MORIN <[email protected]>

Cc Arnout Vandecappelle <[email protected]>

Acked-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

---
Changes v5 -> v6:
- slightly tweak the phrasing (Arnout)

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

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.

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.

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Arnout Vandecappelle <[email protected]>

Reviewed-by Arnout Vandecappelle (Essensium/Mind) <[email protected]>

Reviewed-by Luca Ceresoli <[email protected]>

Tested-by Luca Ceresoli <[email protected]>

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

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

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...

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Luca Ceresoli <[email protected]>

Cc Thomas Petazzoni <[email protected]>

Cc Peter Korsgaard <[email protected]>

Tested-by Luca Ceresoli <[email protected]>

---
Notes: here is a case where one would need to be able to do an off-line legal-info:
- a build farm (e.g. Jenkins slaves) without access to the internet;
- a single machine (not part of the farm) has access to the internet;
- that machine runs "make source" to populate a mirror (a "primary mirror" or an NFS-mounted directory or anything else) that is accessible to the build farm;
- machines in the build farm need the actual sources to run legal-info, doing so off-line.

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.

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.

Quoted 3 lines

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.

The series is now fully applied. Thanks a lot!