[Buildroot] [PATCH 01/13 v6] support/scripts: add helper to hardlink-or-copy

[Buildroot] [PATCH 01/13 v6] support/scripts: add helper to hardlink-or-copy

[Yann E. MORIN]

This helper 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, so if a third argument is specified,
it is treated as the basename of the destination file.

Add a make variable to easily call that script.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Luca Ceresoli <luca@lucaceresoli.net>

---
Changes v5 -> v6:
  - rename script  (Thomas, Arnout)
  - drop the macro  (Thomas, Arnout)
  - drop Luca's reviewed-by and tested-by tags, because the macro
    disapeared
  - fix redundancy in comments in the script

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             |  4 ++++
 support/scripts/hardlink-or-copy | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)
 create mode 100755 support/scripts/hardlink-or-copy

diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk
index f88313a..2692576 100644
--- a/package/pkg-utils.mk
+++ b/package/pkg-utils.mk
@@ -113,6 +113,10 @@ $$(error Package error: use $(2) instead of $(1). Please fix your .mk file)
 endif
 endef
 
+# hardlink-or-copy -- helper script to first try to hardlink,
+# and fallback to copy.
+HARDLINK_OR_COPY := support/scripts/hardlink-or-copy
+
 #
 # legal-info helper functions
 #
diff --git a/support/scripts/hardlink-or-copy b/support/scripts/hardlink-or-copy
new file mode 100755
index 0000000..b581c51
--- /dev/null
+++ b/support/scripts/hardlink-or-copy
@@ -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 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

[Buildroot] [PATCH 02/13 v6] core/legal-info: use the helper to install source archives

[Yann E. MORIN]

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

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Luca Ceresoli <luca@lucaceresoli.net>

---
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 | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index 3904c09..5c2e46d 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -803,9 +803,9 @@ 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)$$(HARDLINK_OR_COPY) $$(DL_DIR)/$$($(2)_ACTUAL_SOURCE_TARBALL),\
+				  $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4)))
 endif # redistribute
 
 endif # other packages
-- 
1.9.1

[Buildroot] [PATCH 03/13 v6] core/pkg-generic: reorder variables definitions for legal-info

[Yann E. MORIN]

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" <yann.morin.1998@free.fr>
Cc: Arnout Vandecappelle <arnout@mind.be>
Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
Tested-by: Luca Ceresoli <luca@lucaceresoli.net>

---
Changes v2 -> v3:
  - move that to its own patch  (Arnout)
---
 package/pkg-generic.mk | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index 5c2e46d..cbdf96e 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -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

[Buildroot] [PATCH 04/13 v6] core/legal-info: ensure legal-info works in off-line mode

[Yann E. MORIN]

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" <yann.morin.1998@free.fr>
Cc: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Peter Korsgaard <jacmet@uclibc.org>
Tested-by: Luca Ceresoli <luca@lucaceresoli.net>

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

So, "make source" has to be complete, i.e. it must also download the
acutal source archives.

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

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index cbdf96e..2c405e1 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -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)$$(HARDLINK_OR_COPY) $$(DL_DIR)/$$($(2)_ACTUAL_SOURCE_TARBALL),\
 				  $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4)))
@@ -896,6 +914,7 @@ endif
 	$(1)-install-staging \
 	$(1)-install-target \
 	$(1)-legal-info \
+	$(1)-legal-source \
 	$(1)-patch \
 	$(1)-rebuild \
 	$(1)-reconfigure \
-- 
1.9.1

[Buildroot] [PATCH 00/13 v6] legal-info improvements and completeness (branch yem/legal-3)

[Yann E. MORIN]

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.

TL;DR:

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


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

  - patches 1-4 make sure legal-info will work in off-line mode;

  - patches 5-7 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 8-11 actually save the patches and extra downloads in the
    legal-info output;

  - patch 12 adds a list of hashes for all files in the legal-info
    output;

  - patch 15 (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 second solution. The immediate drawback is
that in rare circumstances, existing setups (e.g. with patches in a
global patch directory) which has duplicatre basenames will cease to
work at the time patches are applied.

that most patches will have two indexes, while the immediate advantage
is that it accepts input patches with the same basename.

The alternative first solution, although tested and implemented (and
previously submitted) has the drawback that most patches will have two
indexes, whithe the immediate advantage that it accepts input patches
with the same basename (and thus would not break existing setups).

After the previous reviews, it was deemed preferrable to have less ugly
patch filenames and break the build, rather than have uglier patch
filenames and not break the build, given how unlikely the situation with
duplicate basenames is.


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.


Changlog of the series
----------------------

Changes v5 -> v6:
  - drop the hardlink-or-copy macro, only keep the script  (Thomas,
    Arnout)
  - don;t renumber patches; instead, detect duplicate files basenames
    and abort  (Thomas, Arnout, Luca)
  - rename the raw basename variable  (Thomas)
  - tweak the phrasing about patches licenses  (Arnout)
  - completely ignoring packages from legal-info was deemd too
    controversial; drop it (at least for now! ;-) )
  - fix misc comments and typoes

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 bbe29b9896d5a6a3b10c999522bb0ac29e934b51:

  purge-locales: Handle empty locale directories better (2016-04-28 23:48:09 +0200)

are available in the git repository at:

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

for you to fetch changes up to d7b10938001e539340479d97a2b64f4ab49d3726:

  legal-info: explicitly state how patches are licensed (2016-04-29 00:07:26 +0200)

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

Yann E. MORIN (12):
      support/scripts: add helper to hardlink-or-copy
      core/legal-info: use the helper 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
      support/apply-patches: bail-out on duplicate patch basenames
      core/legal-info: also save extra downloads
      core/legal-info: generate a hash of all saved files

 Makefile                         |  8 ++++-
 package/pkg-generic.mk           | 73 +++++++++++++++++++++++++++-------------
 package/pkg-utils.mk             |  4 +++
 support/legal-info/README.header | 10 +++---
 support/scripts/apply-patches.sh | 17 ++++++++--
 support/scripts/hardlink-or-copy | 34 +++++++++++++++++++
 6 files changed, 115 insertions(+), 31 deletions(-)
 create mode 100755 support/scripts/hardlink-or-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.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 05/13 v6] core/pkg-generic: add variable to store the package rawname-version

[Yann E. MORIN]

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 <luca@lucaceresoli.net>
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Luca Ceresoli <luca@lucaceresoli.net>
Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

---
Changes v5 -> v6:
  - rename variable  (Thomas)
---
 package/pkg-generic.mk | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index 2c405e1..7aca958 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -399,6 +399,7 @@ ifdef $(3)_OVERRIDE_SRCDIR
 endif
 
 $(2)_BASE_NAME	=  $(1)-$$($(2)_VERSION)
+$(2)_RAW_BASE_NAME = $$($(2)_RAWNAME)-$$($(2)_VERSION)
 $(2)_DL_DIR	=  $$(DL_DIR)/$$($(2)_BASE_NAME)
 $(2)_DIR	=  $$(BUILD_DIR)/$$($(2)_BASE_NAME)
 
@@ -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)_RAW_BASE_NAME).tar.gz
  endif
 endif
 
-- 
1.9.1

[Buildroot] [PATCH 06/13 v6] core/legal-info: install source archives in their own sub-dir

[Yann E. MORIN]

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" <yann.morin.1998@free.fr>
Cc: Luca Ceresoli <luca@lucaceresoli.net>
Acked-by: Luca Ceresoli <luca@lucaceresoli.net>
Tested-by: Luca Ceresoli <luca@lucaceresoli.net>
Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
Reviewed-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

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

Changes v1 -> v2:
  - perl no longer has a post-legal-info hook  (Thomas, Luca)
---
 package/pkg-generic.mk | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index 7aca958..a362b73 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -496,6 +496,8 @@ endif
 
 $(2)_REDISTRIBUTE		?= YES
 
+$(2)_REDIST_SOURCES_DIR = $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4)))/$$($(2)_RAW_BASE_NAME)
+
 # 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
@@ -823,8 +825,8 @@ else
 
 ifeq ($$($(2)_REDISTRIBUTE),YES)
 # Save the source tarball
-	$$(Q)$$(HARDLINK_OR_COPY) $$(DL_DIR)/$$($(2)_ACTUAL_SOURCE_TARBALL),\
-				  $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4)))
+	$$(Q)$$(HARDLINK_OR_COPY) $$(DL_DIR)/$$($(2)_ACTUAL_SOURCE_TARBALL) \
+				  $$($(2)_REDIST_SOURCES_DIR)
 endif # redistribute
 
 endif # other packages
-- 
1.9.1

[Buildroot] [PATCH 07/13 v6] core/legal-info: add package version to license directory

[Yann E. MORIN]

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" <yann.morin.1998@free.fr>
Cc: Luca Ceresoli <luca@lucaceresoli.net>
Acked-by: Luca Ceresoli <luca@lucaceresoli.net>
Tested-by: Luca Ceresoli <luca@lucaceresoli.net>

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

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index a362b73..f48c58b 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -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)_RAW_BASE_NAME),$$(call UPPERCASE,$(4)))
+	@$$(call legal-warning-pkg,$$($(2)_RAW_BASE_NAME),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)_RAW_BASE_NAME),$$(F),$$($(2)_DIR)/$$(F),$$(call UPPERCASE,$(4)))$$(sep))
 endif # license files
 
 ifeq ($$($(2)_SITE_METHOD),local)
-- 
1.9.1

[Buildroot] [PATCH 08/13 v6] core/apply-patches: store full path of applied patches

[Yann E. MORIN]

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" <yann.morin.1998@free.fr>
Cc: Luca Ceresoli <luca@lucaceresoli.net>
Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
[Tested only with patches in the Buildroot sources]
Tested-by: Luca Ceresoli <luca@lucaceresoli.net>

---
Changes v1 -> v2:
  - do not duplicate '/' in paths  (Luca)
---
 support/scripts/apply-patches.sh | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/support/scripts/apply-patches.sh b/support/scripts/apply-patches.sh
index 201278d..20a1552 100755
--- a/support/scripts/apply-patches.sh
+++ b/support/scripts/apply-patches.sh
@@ -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

[Buildroot] [PATCH 09/13 v6] core/legal-info: also save patches

[Yann E. MORIN]

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" <yann.morin.1998@free.fr>
Cc: Luca Ceresoli <luca@lucaceresoli.net>
Tested-by: Luca Ceresoli <luca@lucaceresoli.net>
Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>

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

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index f48c58b..65c0db5 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -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).
@@ -827,6 +825,11 @@ ifeq ($$($(2)_REDISTRIBUTE),YES)
 # Save the source tarball
 	$$(Q)$$(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 \
+		$$(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
 endif # redistribute
 
 endif # other packages
diff --git a/support/legal-info/README.header b/support/legal-info/README.header
index d07c45d..418de14 100644
--- a/support/legal-info/README.header
+++ b/support/legal-info/README.header
@@ -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

[Buildroot] [PATCH 10/13 v6] support/apply-patches: bail-out on duplicate patch basenames

[Yann E. MORIN]

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" <yann.morin.1998@free.fr>
Cc: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Arnout Vandecappelle <arnout@mind.be>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

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

diff --git a/support/scripts/apply-patches.sh b/support/scripts/apply-patches.sh
index 20a1552..e85891a 100755
--- a/support/scripts/apply-patches.sh
+++ b/support/scripts/apply-patches.sh
@@ -103,6 +103,12 @@ 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 ${path}/$patch"
+        echo "Previous one was at: ${existing}"
+        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
-- 
1.9.1

[Buildroot] [PATCH 11/13 v6] core/legal-info: also save extra downloads

[Yann E. MORIN]

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" <yann.morin.1998@free.fr>
Cc: Luca Ceresoli <luca@lucaceresoli.net>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
Tested-by: Luca Ceresoli <luca@lucaceresoli.net>

---
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 | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index 65c0db5..68a33ce 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -822,9 +822,11 @@ else
 # Other packages
 
 ifeq ($$($(2)_REDISTRIBUTE),YES)
-# Save the source tarball
-	$$(Q)$$(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)$$(HARDLINK_OR_COPY) $$(DL_DIR)/$$(e) \
+					  $$($(2)_REDIST_SOURCES_DIR)$$(sep))
 # Save patches and generate the series file
 	$$(Q)while read f; do \
 		$$(HARDLINK_OR_COPY) $$$${f} $$($(2)_REDIST_SOURCES_DIR) || exit 1; \
-- 
1.9.1

[Buildroot] [PATCH 12/13 v6] core/legal-info: generate a hash of all saved files

[Yann E. MORIN]

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.

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Luca Ceresoli <luca@lucaceresoli.net>
Acked-by: Luca Ceresoli <luca@lucaceresoli.net>
Tested-by: Luca Ceresoli <luca@lucaceresoli.net>
Acked-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

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

diff --git a/Makefile b/Makefile
index daa32a4..688bf26 100644
--- a/Makefile
+++ b/Makefile
@@ -701,8 +701,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

[Buildroot] [PATCH 13/13 v6] legal-info: explicitly state how patches are licensed

[Yann E. MORIN]

From: Luca Ceresoli <luca@lucaceresoli.net>

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
Acked-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
[yann.morin.1998 at free.fr: slightyl tweak after Arnout's review]
Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
Cc: Arnout Vandecappelle <arnout@mind.be>

---
Changes v5 -> v6:
  - slightly tweak the phrasing  (Arnout)
---
 support/legal-info/README.header | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/support/legal-info/README.header b/support/legal-info/README.header
index 418de14..1f3524f 100644
--- a/support/legal-info/README.header
+++ b/support/legal-info/README.header
@@ -18,7 +18,8 @@ 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 files that they modify
+   in 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

[Buildroot] [PATCH 00/13 v6] legal-info improvements and completeness (branch yem/legal-3)

[Arnout Vandecappelle]

On 04/29/16 00:27, Yann E. MORIN wrote:
> 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.
>
> TL;DR:
>
> Currently, our legal-info ouput is missing two types of files that might
> be important to have included in the legal-info output:
>   - patches
>   - extra downloads
>
>
> This series is split in 5 consecutive parts, each depending on the
> previous ones:
>
>   - patches 1-4 make sure legal-info will work in off-line mode;
>
>   - patches 5-7 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 8-11 actually save the patches and extra downloads in the
>     legal-info output;
>
>   - patch 12 adds a list of hashes for all files in the legal-info
>     output;
>
>   - patch 15 (from Luca) explicits patches licensing.

  That's patch 13, actually :-)

  For 2016.05, we certainly still want patch 13, and probably also patches 1, 2, 
5, 6, 7, 8, 9, 11 (because they fix a real issue, that legal-info is 
incomplete). I do have some comments on these but it's nitpicking mostly.

  The other patches are more controversial.

  If you would repost, do you think you could take that order?

  Regards,
  Arnout

>
>
> ------------------------------------------------------------------------
>
> 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 second solution. The immediate drawback is
> that in rare circumstances, existing setups (e.g. with patches in a
> global patch directory) which has duplicatre basenames will cease to
> work at the time patches are applied.
>
> that most patches will have two indexes, while the immediate advantage
> is that it accepts input patches with the same basename.
>
> The alternative first solution, although tested and implemented (and
> previously submitted) has the drawback that most patches will have two
> indexes, whithe the immediate advantage that it accepts input patches
> with the same basename (and thus would not break existing setups).
>
> After the previous reviews, it was deemed preferrable to have less ugly
> patch filenames and break the build, rather than have uglier patch
> filenames and not break the build, given how unlikely the situation with
> duplicate basenames is.
>
>
> 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.
>
>
> Changlog of the series
> ----------------------
>
> Changes v5 -> v6:
>   - drop the hardlink-or-copy macro, only keep the script  (Thomas,
>     Arnout)
>   - don;t renumber patches; instead, detect duplicate files basenames
>     and abort  (Thomas, Arnout, Luca)
>   - rename the raw basename variable  (Thomas)
>   - tweak the phrasing about patches licenses  (Arnout)
>   - completely ignoring packages from legal-info was deemd too
>     controversial; drop it (at least for now! ;-) )
>   - fix misc comments and typoes
>
> 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 bbe29b9896d5a6a3b10c999522bb0ac29e934b51:
>
>   purge-locales: Handle empty locale directories better (2016-04-28 23:48:09 +0200)
>
> are available in the git repository at:
>
>   git://git.busybox.net/~ymorin/git/buildroot yem/legal-3
>
> for you to fetch changes up to d7b10938001e539340479d97a2b64f4ab49d3726:
>
>   legal-info: explicitly state how patches are licensed (2016-04-29 00:07:26 +0200)
>
> ----------------------------------------------------------------
> Luca Ceresoli (1):
>       legal-info: explicitly state how patches are licensed
>
> Yann E. MORIN (12):
>       support/scripts: add helper to hardlink-or-copy
>       core/legal-info: use the helper 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
>       support/apply-patches: bail-out on duplicate patch basenames
>       core/legal-info: also save extra downloads
>       core/legal-info: generate a hash of all saved files
>
>  Makefile                         |  8 ++++-
>  package/pkg-generic.mk           | 73 +++++++++++++++++++++++++++-------------
>  package/pkg-utils.mk             |  4 +++
>  support/legal-info/README.header | 10 +++---
>  support/scripts/apply-patches.sh | 17 ++++++++--
>  support/scripts/hardlink-or-copy | 34 +++++++++++++++++++
>  6 files changed, 115 insertions(+), 31 deletions(-)
>  create mode 100755 support/scripts/hardlink-or-copy
>


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

[Buildroot] [PATCH 13/13 v6] legal-info: explicitly state how patches are licensed

[Arnout Vandecappelle]

On 04/29/16 00:27, Yann E. MORIN wrote:
> From: Luca Ceresoli <luca@lucaceresoli.net>
>
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> Acked-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> [yann.morin.1998 at free.fr: slightyl tweak after Arnout's review]

  slightly :-)

Acked-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

  Regards,
  Arnout

> Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
> Cc: Arnout Vandecappelle <arnout@mind.be>
>
> ---
> Changes v5 -> v6:
>   - slightly tweak the phrasing  (Arnout)
> ---
>  support/legal-info/README.header | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/support/legal-info/README.header b/support/legal-info/README.header
> index 418de14..1f3524f 100644
> --- a/support/legal-info/README.header
> +++ b/support/legal-info/README.header
> @@ -18,7 +18,8 @@ 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 files that they modify
> +   in 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.
>


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

[Buildroot] [PATCH 01/13 v6] support/scripts: add helper to hardlink-or-copy

[Arnout Vandecappelle]

On 04/29/16 00:27, Yann E. MORIN wrote:
> This helper 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, so if a third argument is specified,
> it is treated as the basename of the destination file.
>
> Add a make variable to easily call that script.
>
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Luca Ceresoli <luca@lucaceresoli.net>

  I agree that it's a good idea to move this to a script. I have a few coding 
style issues with it, but it's in things for we don't have a set style anyway so 
my word is not law :-). There is only one thing really wrong...

>
> ---
> Changes v5 -> v6:
>   - rename script  (Thomas, Arnout)
>   - drop the macro  (Thomas, Arnout)
>   - drop Luca's reviewed-by and tested-by tags, because the macro
>     disapeared
>   - fix redundancy in comments in the script
>
> 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             |  4 ++++
>  support/scripts/hardlink-or-copy | 34 ++++++++++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+)
>  create mode 100755 support/scripts/hardlink-or-copy
>
> diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk
> index f88313a..2692576 100644
> --- a/package/pkg-utils.mk
> +++ b/package/pkg-utils.mk
> @@ -113,6 +113,10 @@ $$(error Package error: use $(2) instead of $(1). Please fix your .mk file)
>  endif
>  endef
>
> +# hardlink-or-copy -- helper script to first try to hardlink,
> +# and fallback to copy.
> +HARDLINK_OR_COPY := support/scripts/hardlink-or-copy

  = instead of :=


  However (and here starts the difference of opinion in coding style): why do 
you need to define a variable for this? In general, I'm not a big fan of 
variables that just contain a constant string. For numbers, it makes sense, 
because a number is meaningless by itself. But a string is already something 
that is searchable and identifiable. And in this specific case, the variable 
name is even the same as the value... So what's the point? OK, you save 13 
characters in every place where you use it (that's in only 2 (TWO) places, by 
the way).

  We do have a few existing cases of such variables, but IMHO they are equally 
useless. And mostly introduced by you, BTW :-)


> +
>  #
>  # legal-info helper functions
>  #
> diff --git a/support/scripts/hardlink-or-copy b/support/scripts/hardlink-or-copy
> new file mode 100755
> index 0000000..b581c51
> --- /dev/null
> +++ b/support/scripts/hardlink-or-copy
> @@ -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 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}"

  Personally I prefer my variable names to be more explicit: src_file

> +    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}" )"

  I thought you were more into ${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}"

  Since there's an rm just above, is there any point in adding the -f still 
here? Yes, for race conditions, but then the rm wasn't much use to begin with.

> +}
> +
> +main "${@}"

  Anothing coding style disagreement. What's the point of putting your complete 
shell script in a function? I don't really have arguments against it, but do you 
have reasons why you think it's better like that?

  Note that currently we have both styles of shell scripts, though the ones with 
a main function are a minority. And none of the python scripts have a main.

  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

[Buildroot] [PATCH 02/13 v6] core/legal-info: use the helper to install source archives

[Arnout Vandecappelle]

On 04/29/16 00:27, Yann E. MORIN wrote:
> .. and use $(Q) instead of a hard-coded @.
>
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Cc: Luca Ceresoli <luca@lucaceresoli.net>
>
> ---
> 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 | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index 3904c09..5c2e46d 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -803,9 +803,9 @@ 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)$$(HARDLINK_OR_COPY) $$(DL_DIR)/$$($(2)_ACTUAL_SOURCE_TARBALL),\

  Did you test this? I don't think it can work with that comma... Ah, but it 
gets fixed in patch 6 :-)

  Regards,
  Arnout

> +				  $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4)))
>  endif # redistribute
>
>  endif # other packages
>


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

[Buildroot] [PATCH 04/13 v6] core/legal-info: ensure legal-info works in off-line mode

[Arnout Vandecappelle]

On 04/29/16 00:27, Yann E. MORIN wrote:
> 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" <yann.morin.1998@free.fr>
> Cc: Luca Ceresoli <luca@lucaceresoli.net>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Peter Korsgaard <jacmet@uclibc.org>
> Tested-by: Luca Ceresoli <luca@lucaceresoli.net>
>
> ---
> 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.
>
> So, "make source" has to be complete, i.e. it must also download the
> acutal source archives.

  Although I agree that this a valid and worthwhile use case, I'm not convinced 
that it's worth adding 20 lines of code. Up to Peter to decide here :-)

  But I really do like the way it's handled here: in normal builds, the actual 
source will not be downloaded; only for legal-info and 'make source' it will be 
there. Perfect.

>
> ---
> 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(-)
>
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index cbdf96e..2c405e1 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -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

  Instead of adding that here, I think it makes more sense to put these 
dependencies where we already have them for -source, so:

$(1)-all-source:        $(1)-source $(1)-legal-source
...
# Packages that have a tarball need it downloaded beforehand
$(1)-legal-info: $(1)-source $(1)-legal-source $$(REDIST_SOURCES_DIR_$$(call 
UPPERCASE,$(4)))

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

  This is not actually needed because it's already declared PHONY. The -source 
bit is a redundant leftover. That said, for symmetry it's probably best to keep 
it there.

>
>  $(1)-source-check:

  Hm, what about source-check?

  Well, we should probably remove the source-check feature :-) It's a lot of 
code for something that is hardly used IMHO.


  Regards,
  Arnout

>  	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)$$(HARDLINK_OR_COPY) $$(DL_DIR)/$$($(2)_ACTUAL_SOURCE_TARBALL),\
>  				  $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4)))
> @@ -896,6 +914,7 @@ endif
>  	$(1)-install-staging \
>  	$(1)-install-target \
>  	$(1)-legal-info \
> +	$(1)-legal-source \
>  	$(1)-patch \
>  	$(1)-rebuild \
>  	$(1)-reconfigure \
>


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

[Buildroot] [PATCH 06/13 v6] core/legal-info: install source archives in their own sub-dir

[Arnout Vandecappelle]

On 04/29/16 00:27, Yann E. MORIN wrote:
> 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" <yann.morin.1998@free.fr>
> Cc: Luca Ceresoli <luca@lucaceresoli.net>
> Acked-by: Luca Ceresoli <luca@lucaceresoli.net>
> Tested-by: Luca Ceresoli <luca@lucaceresoli.net>
> Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> Reviewed-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
>
> ---
> Chages v5 -> v6:
>   - variable with raw name was renamed  (Thomas)
>
> Changes v1 -> v2:
>   - perl no longer has a post-legal-info hook  (Thomas, Luca)
> ---
>  package/pkg-generic.mk | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index 7aca958..a362b73 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -496,6 +496,8 @@ endif
>
>  $(2)_REDISTRIBUTE		?= YES
>
> +$(2)_REDIST_SOURCES_DIR = $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4)))/$$($(2)_RAW_BASE_NAME)
> +
>  # 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
> @@ -823,8 +825,8 @@ else
>
>  ifeq ($$($(2)_REDISTRIBUTE),YES)
>  # Save the source tarball
> -	$$(Q)$$(HARDLINK_OR_COPY) $$(DL_DIR)/$$($(2)_ACTUAL_SOURCE_TARBALL),\
> -				  $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4)))
> +	$$(Q)$$(HARDLINK_OR_COPY) $$(DL_DIR)/$$($(2)_ACTUAL_SOURCE_TARBALL) \

  Did I really give my Rev-by without noticing that comma disappearing? :-)

  Regards,
  Arnout

> +				  $$($(2)_REDIST_SOURCES_DIR)
>  endif # redistribute
>
>  endif # other packages
>


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

[Buildroot] [PATCH 00/13 v6] legal-info improvements and completeness (branch yem/legal-3)

[Yann E. MORIN]

Arnout, All,

On 2016-05-04 00:33 +0200, Arnout Vandecappelle spake thusly:
> On 04/29/16 00:27, Yann E. MORIN wrote:
> >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.
> >
> >TL;DR:
> >
> >Currently, our legal-info ouput is missing two types of files that might
> >be important to have included in the legal-info output:
> >  - patches
> >  - extra downloads
> >
> >
> >This series is split in 5 consecutive parts, each depending on the
> >previous ones:
> >
> >  - patches 1-4 make sure legal-info will work in off-line mode;
> >
> >  - patches 5-7 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 8-11 actually save the patches and extra downloads in the
> >    legal-info output;
> >
> >  - patch 12 adds a list of hashes for all files in the legal-info
> >    output;
> >
> >  - patch 15 (from Luca) explicits patches licensing.
> 
>  That's patch 13, actually :-)

Doh... Are you sure?  ;-)

/me looks at his keyboard. '3' and '5' are not side-by-side; there is
'4' in-between... Damn, so far for my kbd-dislexia... ;-)

>  For 2016.05, we certainly still want patch 13, and probably also patches 1,
> 2, 5, 6, 7, 8, 9, 11 (because they fix a real issue, that legal-info is
> incomplete). I do have some comments on these but it's nitpicking mostly.
> 
>  The other patches are more controversial.

As much as I can see that patch 4 (and 3) might be controversial, I fail
to see why patch 10 (bail-out when two patches are named the same) is
still controversial.

For that patch, I switched from renaming to bailing out after both
Thomas, Luca and you commented that was what you prefered.

I also stated that I had implemented both solutions, but posted the
renaming one so as to not break existing system. But surely, if I had
both solutions, it was for a reason: I also prefer bailing out. ;-)

So, I fail to see the controversy on that patch. ;-)

As for 12, Thomas and Luca already acked it...

>  If you would repost, do you think you could take that order?

I doubt it...

I can try, but if I face a rebase hell, I'll bail out...

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

[Buildroot] [PATCH 01/13 v6] support/scripts: add helper to hardlink-or-copy

[Yann E. MORIN]

Arnout, All,

On 2016-05-04 00:53 +0200, Arnout Vandecappelle spake thusly:
> On 04/29/16 00:27, Yann E. MORIN wrote:
> >This helper 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, so if a third argument is specified,
> >it is treated as the basename of the destination file.
> >
> >Add a make variable to easily call that script.
> >
> >Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> >Cc: Luca Ceresoli <luca@lucaceresoli.net>
> 
>  I agree that it's a good idea to move this to a script. I have a few coding
> style issues with it, but it's in things for we don't have a set style
> anyway so my word is not law :-). There is only one thing really wrong...
> 
> >
> >---
> >Changes v5 -> v6:
> >  - rename script  (Thomas, Arnout)
> >  - drop the macro  (Thomas, Arnout)
> >  - drop Luca's reviewed-by and tested-by tags, because the macro
> >    disapeared
> >  - fix redundancy in comments in the script
> >
> >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             |  4 ++++
> > support/scripts/hardlink-or-copy | 34 ++++++++++++++++++++++++++++++++++
> > 2 files changed, 38 insertions(+)
> > create mode 100755 support/scripts/hardlink-or-copy
> >
> >diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk
> >index f88313a..2692576 100644
> >--- a/package/pkg-utils.mk
> >+++ b/package/pkg-utils.mk
> >@@ -113,6 +113,10 @@ $$(error Package error: use $(2) instead of $(1). Please fix your .mk file)
> > endif
> > endef
> >
> >+# hardlink-or-copy -- helper script to first try to hardlink,
> >+# and fallback to copy.
> >+HARDLINK_OR_COPY := support/scripts/hardlink-or-copy
> 
>  = instead of :=
> 
> 
>  However (and here starts the difference of opinion in coding style): why do
> you need to define a variable for this? In general, I'm not a big fan of
> variables that just contain a constant string. For numbers, it makes sense,
> because a number is meaningless by itself. But a string is already something
> that is searchable and identifiable. And in this specific case, the variable
> name is even the same as the value... So what's the point? OK, you save 13
> characters in every place where you use it (that's in only 2 (TWO) places,
> by the way).
> 
>  We do have a few existing cases of such variables, but IMHO they are
> equally useless. And mostly introduced by you, BTW :-)

Well, when the thing s used in multiple places, I find it "better" to
use a variable so as to be able to change all locations at once with a
single edit (even if we don't plan on renaming or moving it).

> > #
> > # legal-info helper functions
> > #
> >diff --git a/support/scripts/hardlink-or-copy b/support/scripts/hardlink-or-copy
> >new file mode 100755
> >index 0000000..b581c51
> >--- /dev/null
> >+++ b/support/scripts/hardlink-or-copy
> >@@ -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 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}"
> 
>  Personally I prefer my variable names to be more explicit: src_file

OK

> >+    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}" )"
> 
>  I thought you were more into ${src_f##*/}?

Yes, but when I don't get my two cups of cofee in the morning, I can
write insanities like the above! ;-)

But seriously, it's inherited from the Makefile macro, which sould not
use the shell substituion.

I'll fix. ;-]

> >+    fi
> >+
> >+    mkdir -p "${dst_d}"
> >+    rm -f "${dst_f}"
> >+    ln -f "${src_f}" "${dst_f}" 2>/dev/null || cp -f "${src_f}" "${dst_f}"
> 
>  Since there's an rm just above, is there any point in adding the -f still
> here? Yes, for race conditions, but then the rm wasn't much use to begin
> with.

That's what was diuscussed in the previous reviews. And didn't you
explcitly mentioned we should use an explicit rm -f :

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

;-)

> >+}
> >+
> >+main "${@}"
> 
>  Anothing coding style disagreement. What's the point of putting your
> complete shell script in a function? I don't really have arguments against
> it, but do you have reasons why you think it's better like that?

I am not sure what advantages it has, technically-wise (i.e. I am not
sure it allows things that would not be possible without main).

I've been writing (bash) shell scripts for ages now, and I find it very
cnvenient to use a main():

  - do not use global variables (even though variables are inherited by
    sub-functions): declare all variables used in the function and only
    use that, with explciitly docuemented exceptions;

  - it helps organise the code: it is cleaner in my head;

  - I'm not the only one: https://google.github.io/styleguide/shell.xml

>  Note that currently we have both styles of shell scripts, though the ones
> with a main function are a minority.

... and probably mines. ;-)

Regards,
Yann E. MORIN.

> And none of the python scripts have a
> main.
> 
>  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

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

[Buildroot] [PATCH 02/13 v6] core/legal-info: use the helper to install source archives

[Yann E. MORIN]

Arnout, All,

On 2016-05-04 00:57 +0200, Arnout Vandecappelle spake thusly:
> On 04/29/16 00:27, Yann E. MORIN wrote:
> >.. and use $(Q) instead of a hard-coded @.
> >
> >Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> >Cc: Luca Ceresoli <luca@lucaceresoli.net>
> >
> >---
> >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 | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> >diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> >index 3904c09..5c2e46d 100644
> >--- a/package/pkg-generic.mk
> >+++ b/package/pkg-generic.mk
> >@@ -803,9 +803,9 @@ 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)$$(HARDLINK_OR_COPY) $$(DL_DIR)/$$($(2)_ACTUAL_SOURCE_TARBALL),\
> 
>  Did you test this? I don't think it can work with that comma...

Wooppsss... :-/

> Ah, but it gets fixed in patch 6 :-)

Re-wooppsss... :-/

Thanks! :-)

Regards,
Yann E. MORIN.

>  Regards,
>  Arnout
> 
> >+				  $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4)))
> > endif # redistribute
> >
> > endif # other packages
> >
> 
> 
> -- 
> 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

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

[Buildroot] [PATCH 06/13 v6] core/legal-info: install source archives in their own sub-dir

[Yann E. MORIN]

Arnout, All,

On 2016-05-04 01:14 +0200, Arnout Vandecappelle spake thusly:
> On 04/29/16 00:27, Yann E. MORIN wrote:
> >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" <yann.morin.1998@free.fr>
> >Cc: Luca Ceresoli <luca@lucaceresoli.net>
> >Acked-by: Luca Ceresoli <luca@lucaceresoli.net>
> >Tested-by: Luca Ceresoli <luca@lucaceresoli.net>
> >Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> >Reviewed-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> >
> >---
> >Chages v5 -> v6:
> >  - variable with raw name was renamed  (Thomas)
> >
> >Changes v1 -> v2:
> >  - perl no longer has a post-legal-info hook  (Thomas, Luca)
> >---
> > package/pkg-generic.mk | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> >diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> >index 7aca958..a362b73 100644
> >--- a/package/pkg-generic.mk
> >+++ b/package/pkg-generic.mk
> >@@ -496,6 +496,8 @@ endif
> >
> > $(2)_REDISTRIBUTE		?= YES
> >
> >+$(2)_REDIST_SOURCES_DIR = $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4)))/$$($(2)_RAW_BASE_NAME)
> >+
> > # 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
> >@@ -823,8 +825,8 @@ else
> >
> > ifeq ($$($(2)_REDISTRIBUTE),YES)
> > # Save the source tarball
> >-	$$(Q)$$(HARDLINK_OR_COPY) $$(DL_DIR)/$$($(2)_ACTUAL_SOURCE_TARBALL),\
> >-				  $$(REDIST_SOURCES_DIR_$$(call UPPERCASE,$(4)))
> >+	$$(Q)$$(HARDLINK_OR_COPY) $$(DL_DIR)/$$($(2)_ACTUAL_SOURCE_TARBALL) \
> 
>  Did I really give my Rev-by without noticing that comma disappearing? :-)

No, you did not; it's a tag from a previous review.

I dropped some tags from some patches, but forgot to do so here. Sorry...

Regards,
Yann E. MORIN.

>  Regards,
>  Arnout
> 
> >+				  $$($(2)_REDIST_SOURCES_DIR)
> > endif # redistribute
> >
> > endif # other packages
> >
> 
> 
> -- 
> 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

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

[Buildroot] [PATCH 01/13 v6] support/scripts: add helper to hardlink-or-copy

[Arnout Vandecappelle]

On 05/06/16 00:38, Yann E. MORIN wrote:
> Arnout, All,
>
> On 2016-05-04 00:53 +0200, Arnout Vandecappelle spake thusly:
>> On 04/29/16 00:27, Yann E. MORIN wrote:
[snip]
>>> +# hardlink-or-copy -- helper script to first try to hardlink,
>>> +# and fallback to copy.
>>> +HARDLINK_OR_COPY := support/scripts/hardlink-or-copy
>>
>>  = instead of :=
>>
>>
>>  However (and here starts the difference of opinion in coding style): why do
>> you need to define a variable for this? In general, I'm not a big fan of
>> variables that just contain a constant string. For numbers, it makes sense,
>> because a number is meaningless by itself. But a string is already something
>> that is searchable and identifiable. And in this specific case, the variable
>> name is even the same as the value... So what's the point? OK, you save 13
>> characters in every place where you use it (that's in only 2 (TWO) places,
>> by the way).
>>
>>  We do have a few existing cases of such variables, but IMHO they are
>> equally useless. And mostly introduced by you, BTW :-)
>
> Well, when the thing s used in multiple places, I find it "better" to
> use a variable so as to be able to change all locations at once with a
> single edit (even if we don't plan on renaming or moving it).

  I was going to build a big argument about why I disagree, but I see in v7 you 
already dropped the variable so let's leave it at that :-)

[snip]
>>> +    rm -f "${dst_f}"
>>> +    ln -f "${src_f}" "${dst_f}" 2>/dev/null || cp -f "${src_f}" "${dst_f}"
>>
>>  Since there's an rm just above, is there any point in adding the -f still
>> here? Yes, for race conditions, but then the rm wasn't much use to begin
>> with.
>
> That's what was diuscussed in the previous reviews. And didn't you
> explcitly mentioned we should use an explicit rm -f :
>
>     http://lists.busybox.net/pipermail/buildroot/2016-February/151140.html

  This one you didn't change in v7... The rm -f is perfect; it's the -f of ln/cp 
which is redundant.

  But it's not really important, so I'll probably just Rev-by the v7.

>
> ;-)
>
>>> +}
>>> +
>>> +main "${@}"
>>
>>  Anothing coding style disagreement. What's the point of putting your
>> complete shell script in a function? I don't really have arguments against
>> it, but do you have reasons why you think it's better like that?
>
> I am not sure what advantages it has, technically-wise (i.e. I am not
> sure it allows things that would not be possible without main).
>
> I've been writing (bash) shell scripts for ages now, and I find it very
> cnvenient to use a main():
>
>   - do not use global variables (even though variables are inherited by
>     sub-functions): declare all variables used in the function and only
>     use that, with explciitly docuemented exceptions;
>
>   - it helps organise the code: it is cleaner in my head;
>
>   - I'm not the only one: https://google.github.io/styleguide/shell.xml

  Great explanation, thanks.

  Regards,
  Arnout


>
>>  Note that currently we have both styles of shell scripts, though the ones
>> with a main function are a minority.
>
> ... and probably mines. ;-)
>
> Regards,
> Yann E. MORIN.
>
>> And none of the python scripts have a
>> main.
>>
>>  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 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

[Buildroot] [PATCH 01/13 v6] support/scripts: add helper to hardlink-or-copy

[Yann E. MORIN]

Arnout, All,

On 2016-05-07 20:44 +0200, Arnout Vandecappelle spake thusly:
> On 05/06/16 00:38, Yann E. MORIN wrote:
> >Arnout, All,
> >
> >On 2016-05-04 00:53 +0200, Arnout Vandecappelle spake thusly:
> >>On 04/29/16 00:27, Yann E. MORIN wrote:
> [snip]
> >>>+# hardlink-or-copy -- helper script to first try to hardlink,
> >>>+# and fallback to copy.
> >>>+HARDLINK_OR_COPY := support/scripts/hardlink-or-copy
> >>
> >> = instead of :=
> >>
> >>
> >> However (and here starts the difference of opinion in coding style): why do
> >>you need to define a variable for this? In general, I'm not a big fan of
> >>variables that just contain a constant string. For numbers, it makes sense,
> >>because a number is meaningless by itself. But a string is already something
> >>that is searchable and identifiable. And in this specific case, the variable
> >>name is even the same as the value... So what's the point? OK, you save 13
> >>characters in every place where you use it (that's in only 2 (TWO) places,
> >>by the way).
> >>
> >> We do have a few existing cases of such variables, but IMHO they are
> >>equally useless. And mostly introduced by you, BTW :-)
> >
> >Well, when the thing s used in multiple places, I find it "better" to
> >use a variable so as to be able to change all locations at once with a
> >single edit (even if we don't plan on renaming or moving it).
> 
>  I was going to build a big argument about why I disagree, but I see in v7
> you already dropped the variable so let's leave it at that :-)

Out of curiosity, what's your argument against having such a variable?

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