[Buildroot] [PATCHv2 0/8] fix scp and reintroduce source-check target

[Buildroot] [PATCHv2 0/8] fix scp and reintroduce source-check target

[Thomas De Schampheleire]

From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>

Hello,

After the overwhelming support for 'source-check' at the Buildroot Developer
Days ;-) , here is a second iteration of this series.

The first commit is actually a bugfix separate from source-check. In v1 of this
series it was also included as the second of two 'scp'-related bugfixes, but it
seems only the first bug-fix was applied at that time, which I hadn't realized
until now.

Patches 2-8 are the actual source-check releated ones.
For git, svn, bzr and cvs, there is no actual implementation which means that
the corresponding download scripts will perform the standard download logic,
which will also verify the validity of the specified revision on the remote
server, but not as optimal as for the other methods.

Best regards,
Thomas

Thomas De Schampheleire (8):
  support/download/scp: fix download with scheme prefix 'scp://'
  support/download: reintroduce 'source-check' target
  support/download/hg: implement source-check
  support/download/wget: implement source-check
  support/download/file: implement source-check
  Config.in: reintroduce BR2_SSH
  support/download/scp: implement source-check
  support/download/{bzr,cvs,git,svn}: highlight unimplemented
    source-check

 Config.in                   |  4 ++
 Makefile                    |  7 +++
 package/pkg-download.mk     | 20 ++++++++
 package/pkg-generic.mk      | 14 +++++-
 support/download/bzr        |  2 +
 support/download/cvs        |  2 +
 support/download/dl-wrapper | 99 ++++++++++++++++++++-----------------
 support/download/file       | 12 ++++-
 support/download/git        |  2 +
 support/download/hg         |  7 +++
 support/download/scp        | 17 +++++++
 support/download/svn        |  2 +
 support/download/wget       |  7 +++
 13 files changed, 149 insertions(+), 46 deletions(-)

-- 
2.19.2

[Buildroot] [PATCHv2 1/8] support/download/scp: fix download with scheme prefix 'scp://'

[Thomas De Schampheleire]

From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>

The scp download helper is broken when the server URL starts with 'scp://'.
Such prefix is used in two situations:
1. to let FOO_SITE point to an scp location without explicitly having to set
   'FOO_SITE_METHOD = scp'

2. when BR2_PRIMARY_SITE or BR2_BACKUP_SITE points to an scp location. In
   this case, there is no equivalent of 'SITE_METHOD'.

Strip out the scheme prefix, similarly to how the 'file' download helper
does it. That helper has the same cases as above.

Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
---
 support/download/scp | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/support/download/scp b/support/download/scp
index 49cfff2b9f..80cf495c4e 100755
--- a/support/download/scp
+++ b/support/download/scp
@@ -34,4 +34,7 @@ _scp() {
     eval ${SCP} "${@}"
 }
 
+# Remove any scheme prefix
+uri="${uri##scp://}"
+
 _scp ${verbose} "${@}" "'${uri}/${filename}'" "'${output}'"
-- 
2.19.2

[Buildroot] [PATCHv2 2/8] support/download: reintroduce 'source-check' target

[Thomas De Schampheleire]

From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>

Commit bf28a165d992681a85b43a886005e669b3cb579b removed the source-check
target to allow easier refactoring of the download infrastructure.
It was partly based on the fact that no-one really used this target.

However, it turns out that there _are_ good uses for it. In a work
environment where many people are submitting changes to a Buildroot
repository, one cannot always blindly trust every change. A typical case
of human error is when bumping the version of a package but forgetting to
upload the tarball to the internal BR2_PRIMARY_SITE or (in case of a
repository) pushing the new changesets to the repository.
If a user cannot directly push to the Buildroot repository but needs to
queue their change via an automatic validation system, that validation
system can use 'make source-check' on the relevant defconfigs to protect
against such errors. A full download would also be possible but takes
longer and unnecessarily uses internal network bandwidth.

With that use case in mind, this commit reintroduces the 'source-check'
target, but embedded in the current situation with a dl-wrapper.  The
changes to the wrapper are minimal (not considering additional indentation).
A new option '-C' means 'check only' and will be passed to the download
backends. If the backend supports the option, no download will happen. If it
does not, then the backend will actually perform a download as a means of
checking that the source exists (a form of graceful degradation). In neither
case, though, hash checking is performed (as the standard case is without
download thus without file to calculate hashes on).

Subsequent commits will actually implement -C in the backends.

Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
---
 Makefile                    |  7 +++
 package/pkg-download.mk     | 19 +++++++
 package/pkg-generic.mk      | 14 +++++-
 support/download/dl-wrapper | 99 ++++++++++++++++++++-----------------
 4 files changed, 94 insertions(+), 45 deletions(-)

diff --git a/Makefile b/Makefile
index 74240ab895..8a7825d44a 100644
--- a/Makefile
+++ b/Makefile
@@ -131,6 +131,7 @@ noconfig_targets := menuconfig nconfig gconfig xconfig config oldconfig randconf
 # (default target is to build), or when MAKECMDGOALS contains
 # something else than one of the nobuild_targets.
 nobuild_targets := source %-source \
+	source-check %-source-check %-all-source-check \
 	legal-info %-legal-info external-deps _external-deps \
 	clean distclean help show-targets graph-depends \
 	%-graph-depends %-show-depends %-show-version \
@@ -796,6 +797,9 @@ target-post-image: $(TARGETS_ROOTFS) target-finalize staging-finalize
 .PHONY: source
 source: $(foreach p,$(PACKAGES),$(p)-all-source)
 
+.PHONY: source-check
+source-check: $(foreach p,$(PACKAGES),$(p)-all-source-check)
+
 .PHONY: _external-deps external-deps
 _external-deps: $(foreach p,$(PACKAGES),$(p)-all-external-deps)
 external-deps:
@@ -1081,6 +1085,8 @@ help:
 	@echo '  <pkg>-dirclean         - Remove <pkg> build directory'
 	@echo '  <pkg>-reconfigure      - Restart the build from the configure step'
 	@echo '  <pkg>-rebuild          - Restart the build from the build step'
+	@echo '  <pkg>-source-check     - Check package for valid download URLs'
+	@echo '  <pkg>-all-source-check - Check package and its dependencies for valid download URLs'
 	$(foreach p,$(HELP_PACKAGES), \
 		@echo $(sep) \
 		@echo '$($(p)_NAME):' $(sep) \
@@ -1100,6 +1106,7 @@ help:
 	@echo
 	@echo 'Miscellaneous:'
 	@echo '  source                 - download all sources needed for offline-build'
+	@echo '  source-check           - check selected packages for valid download URLs'
 	@echo '  external-deps          - list external packages used'
 	@echo '  legal-info             - generate info about license compliance'
 	@echo '  printvars              - dump all the internal variables'
diff --git a/package/pkg-download.mk b/package/pkg-download.mk
index 7cd87c38ff..cc04e316e2 100644
--- a/package/pkg-download.mk
+++ b/package/pkg-download.mk
@@ -106,3 +106,22 @@ define DOWNLOAD
 		-- \
 		$($(PKG)_DL_OPTS)
 endef
+
+define SOURCE_CHECK
+	$(Q)mkdir -p $($(PKG)_DL_DIR)
+	$(Q)$(EXTRA_ENV) $(FLOCK) $(DL_WRAPPER) \
+		-C \
+		-c '$($(PKG)_DL_VERSION)' \
+		-d '$($(PKG)_DL_DIR)' \
+		-D '$(DL_DIR)' \
+		-f '$(notdir $(1))' \
+		-H '$($(PKG)_HASH_FILE)' \
+		-n '$($(PKG)_BASENAME_RAW)' \
+		-N '$($(PKG)_RAWNAME)' \
+		-o '$($(PKG)_DL_DIR)/$(notdir $(1))' \
+		$(if $($(PKG)_GIT_SUBMODULES),-r) \
+		$(DOWNLOAD_URIS) \
+		$(QUIET) \
+		-- \
+		$($(PKG)_DL_OPTS)
+endef
diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index f5cab2b9c2..707996e384 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -780,6 +780,10 @@ $(1)-legal-source:	$$($(2)_TARGET_ACTUAL_SOURCE)
 endif # actual sources != sources
 endif # actual sources != ""
 
+$(1)-source-check: PKG=$(2)
+$(1)-source-check:
+	$$(foreach p,$$($(2)_ALL_DOWNLOADS),$$(call SOURCE_CHECK,$$(p))$$(sep))
+
 $(1)-external-deps:
 	@for p in $$($(2)_SOURCE) $$($(2)_PATCH) $$($(2)_EXTRA_DOWNLOADS) ; do \
 		echo `basename $$$$p` ; \
@@ -804,6 +808,9 @@ $(1)-rsync:		$$($(2)_TARGET_RSYNC)
 $(1)-source:
 $(1)-legal-source:
 
+$(1)-source-check:
+	test -d $$($(2)_OVERRIDE_SRCDIR)
+
 $(1)-external-deps:
 	@echo "file://$$($(2)_OVERRIDE_SRCDIR)"
 endif
@@ -838,6 +845,9 @@ $(1)-graph-rdepends: graph-depends-requirements
 $(1)-all-source:	$(1)-source
 $(1)-all-source:	$$(foreach p,$$($(2)_FINAL_ALL_DEPENDENCIES),$$(p)-all-source)
 
+$(1)-all-source-check:	$(1)-source-check
+$(1)-all-source-check:	$$(foreach p,$$($(2)_FINAL_ALL_DEPENDENCIES),$$(p)-all-source-check)
+
 $(1)-all-external-deps:	$(1)-external-deps
 $(1)-all-external-deps:	$$(foreach p,$$($(2)_FINAL_ALL_DEPENDENCIES),$$(p)-all-external-deps)
 
@@ -1036,6 +1046,7 @@ DL_TOOLS_DEPENDENCIES += $$(call extractor-dependency,$$($(2)_SOURCE))
 	$(1)-all-external-deps \
 	$(1)-all-legal-info \
 	$(1)-all-source \
+	$(1)-all-source-check \
 	$(1)-build \
 	$(1)-clean-for-rebuild \
 	$(1)-clean-for-reconfigure \
@@ -1060,7 +1071,8 @@ DL_TOOLS_DEPENDENCIES += $$(call extractor-dependency,$$($(2)_SOURCE))
 	$(1)-rsync \
 	$(1)-show-depends \
 	$(1)-show-version \
-	$(1)-source
+	$(1)-source \
+	$(1)-source-check
 
 ifneq ($$($(2)_SOURCE),)
 ifeq ($$($(2)_SITE),)
diff --git a/support/download/dl-wrapper b/support/download/dl-wrapper
index 3315bd410e..1ed2e654de 100755
--- a/support/download/dl-wrapper
+++ b/support/download/dl-wrapper
@@ -17,7 +17,7 @@
 # We want to catch any unexpected failure, and exit immediately.
 set -e
 
-export BR_BACKEND_DL_GETOPTS=":hc:d:o:n:N:H:ru:qf:e"
+export BR_BACKEND_DL_GETOPTS=":hc:Cd:o:n:N:H:ru:qf:e"
 
 main() {
     local OPT OPTARG
@@ -25,9 +25,10 @@ main() {
     local -a uris
 
     # Parse our options; anything after '--' is for the backend
-    while getopts ":c:d:D:o:n:N:H:rf:u:q" OPT; do
+    while getopts ":c:Cd:D:o:n:N:H:rf:u:q" OPT; do
         case "${OPT}" in
         c)  cset="${OPTARG}";;
+        C)  checkonly=-C;;
         d)  dl_dir="${OPTARG}";;
         D)  old_dl_dir="${OPTARG}";;
         o)  output="${OPTARG}";;
@@ -46,38 +47,40 @@ main() {
     # Forget our options, and keep only those for the backend
     shift $((OPTIND-1))
 
-    if [ -z "${output}" ]; then
-        error "no output specified, use -o\n"
-    fi
+    if [ -z "${checkonly}" ]; then
+        if [ -z "${output}" ]; then
+            error "no output specified, use -o\n"
+        fi
 
-    # Legacy handling: check if the file already exists in the global
-    # download directory. If it does, hard-link it. If it turns out it
-    # was an incorrect download, we'd still check it below anyway.
-    # If we can neither link nor copy, fallback to doing a download.
-    # NOTE! This is not atomic, is subject to TOCTTOU, but the whole
-    # dl-wrapper runs under an flock, so we're safe.
-    if [ ! -e "${output}" -a -e "${old_dl_dir}/${filename}" ]; then
-        ln "${old_dl_dir}/${filename}" "${output}" || \
-        cp "${old_dl_dir}/${filename}" "${output}" || \
-        true
-    fi
+        # Legacy handling: check if the file already exists in the global
+        # download directory. If it does, hard-link it. If it turns out it
+        # was an incorrect download, we'd still check it below anyway.
+        # If we can neither link nor copy, fallback to doing a download.
+        # NOTE! This is not atomic, is subject to TOCTTOU, but the whole
+        # dl-wrapper runs under an flock, so we're safe.
+        if [ ! -e "${output}" -a -e "${old_dl_dir}/${filename}" ]; then
+            ln "${old_dl_dir}/${filename}" "${output}" || \
+            cp "${old_dl_dir}/${filename}" "${output}" || \
+            true
+        fi
 
-    # If the output file already exists and:
-    # - there's no .hash file: do not download it again and exit promptly
-    # - matches all its hashes: do not download it again and exit promptly
-    # - fails at least one of its hashes: force a re-download
-    # - there's no hash (but a .hash file): consider it a hard error
-    if [ -e "${output}" ]; then
-        if support/download/check-hash ${quiet} "${hfile}" "${output}" "${output##*/}"; then
-            exit 0
-        elif [ ${?} -ne 2 ]; then
-            # Do not remove the file, otherwise it might get re-downloaded
-            # from a later location (i.e. primary -> upstream -> mirror).
-            # Do not print a message, check-hash already did.
-            exit 1
+        # If the output file already exists and:
+        # - there's no .hash file: do not download it again and exit promptly
+        # - matches all its hashes: do not download it again and exit promptly
+        # - fails at least one of its hashes: force a re-download
+        # - there's no hash (but a .hash file): consider it a hard error
+        if [ -e "${output}" ]; then
+            if support/download/check-hash ${quiet} "${hfile}" "${output}" "${output##*/}"; then
+                exit 0
+            elif [ ${?} -ne 2 ]; then
+                # Do not remove the file, otherwise it might get re-downloaded
+                # from a later location (i.e. primary -> upstream -> mirror).
+                # Do not print a message, check-hash already did.
+                exit 1
+            fi
+            rm -f "${output}"
+            warn "Re-downloading '%s'...\n" "${output##*/}"
         fi
-        rm -f "${output}"
-        warn "Re-downloading '%s'...\n" "${output##*/}"
     fi
 
     # Look through all the uris that we were given to download the package
@@ -127,7 +130,7 @@ main() {
                 -f "${filename}" \
                 -u "${uri}" \
                 -o "${tmpf}" \
-                ${quiet} ${recurse} -- "${@}"
+                ${quiet} ${recurse} ${checkonly} -- "${@}"
         then
             # cd back to keep path coherence
             cd "${OLDPWD}"
@@ -138,19 +141,21 @@ main() {
         # cd back to free the temp-dir, so we can remove it later
         cd "${OLDPWD}"
 
-        # Check if the downloaded file is sane, and matches the stored hashes
-        # for that file
-        if support/download/check-hash ${quiet} "${hfile}" "${tmpf}" "${output##*/}"; then
-            rc=0
-        else
-            if [ ${?} -ne 3 ]; then
-                rm -rf "${tmpd}"
-                continue
+        if [ -z "${checkonly}" ]; then
+            # Check if the downloaded file is sane, and matches the stored hashes
+            # for that file
+            if support/download/check-hash ${quiet} "${hfile}" "${tmpf}" "${output##*/}"; then
+                rc=0
+            else
+                if [ ${?} -ne 3 ]; then
+                    rm -rf "${tmpd}"
+                    continue
+                fi
+
+                # the hash file exists and there was no hash to check the file
+                # against
+                rc=1
             fi
-
-            # the hash file exists and there was no hash to check the file
-            # against
-            rc=1
         fi
         download_and_check=1
         break
@@ -163,6 +168,12 @@ main() {
         exit 1
     fi
 
+    # If we only need to check the presence of sources, stop here.
+    # No need to handle output files.
+    if [ -n "${checkonly}" ]; then
+        exit 0
+    fi
+
     # tmp_output is in the same directory as the final output, so we can
     # later move it atomically.
     tmp_output="$(mktemp "${output}.XXXXXX")"
-- 
2.19.2

[Buildroot] [PATCHv2 3/8] support/download/hg: implement source-check

[Thomas De Schampheleire]

From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>

Note: the implementation is different (better) than what used to be in
Buildroot before source-check was removed.

The original implementation:
    hg incoming --force -l1 <URL>
would only verify that the repository exists, not that the requested
revision is present.

An already better implementation is:
    hg incoming --force -l1 -r <revision> <URL>
but compared to the next solution it has a large resource consumption on the
local machine. In the background, the full repository is first downloaded.

The implemented solution is:
    hg identify -r <revision> <URL>
which operates directly on the remote repository.

Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
---
 support/download/hg | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/support/download/hg b/support/download/hg
index efb515fca5..ed8dcfbcff 100755
--- a/support/download/hg
+++ b/support/download/hg
@@ -7,6 +7,7 @@ set -e
 #
 # Options:
 #   -q          Be quiet.
+#   -C          Only check that the changeset exists in the remote repository.
 #   -o FILE     Generate archive in FILE.
 #   -u URI      Clone from repository at URI.
 #   -c CSET     Use changeset (or revision) CSET.
@@ -19,6 +20,7 @@ verbose=
 while getopts "${BR_BACKEND_DL_GETOPTS}" OPT; do
     case "${OPT}" in
     q)  verbose=-q;;
+    C)  checkonly=1;;
     o)  output="${OPTARG}";;
     u)  uri="${OPTARG}";;
     c)  cset="${OPTARG}";;
@@ -36,6 +38,11 @@ _hg() {
     eval ${HG} "${@}"
 }
 
+if [ -n "${checkonly}" ]; then
+    _hg identify ${verbose} "${@}" --rev "'${cset}'" "'${uri}'" > /dev/null
+    exit ${?}
+fi
+
 _hg clone ${verbose} "${@}" --noupdate "'${uri}'" "'${basename}'"
 
 _hg archive ${verbose} --repository "'${basename}'" --type tgz \
-- 
2.19.2

[Buildroot] [PATCHv2 4/8] support/download/wget: implement source-check

[Thomas De Schampheleire]

From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>

Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
---
 support/download/wget | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/support/download/wget b/support/download/wget
index c69e6071aa..7f631ebb61 100755
--- a/support/download/wget
+++ b/support/download/wget
@@ -7,6 +7,7 @@ set -e
 #
 # Options:
 #   -q          Be quiet.
+#   -C          Only check that the file exists remotely.
 #   -o FILE     Save into file FILE.
 #   -f FILENAME The filename of the tarball to get at URL
 #   -u URL      Download file at URL.
@@ -19,6 +20,7 @@ verbose=
 while getopts "${BR_BACKEND_DL_GETOPTS}" OPT; do
     case "${OPT}" in
     q)  verbose=-q;;
+    C)  checkonly=1;;
     o)  output="${OPTARG}";;
     f)  filename="${OPTARG}";;
     u)  url="${OPTARG}";;
@@ -40,4 +42,9 @@ _wget() {
 # mirror
 [ -n "${encode}" ] && filename=${filename//\?/%3F}
 
+if [ -n "${checkonly}" ]; then
+    _wget --spider ${verbose} "${@}" "'${url}/${filename}'"
+    exit ${?}
+fi
+
 _wget ${verbose} "${@}" -O "'${output}'" "'${url}/${filename}'"
-- 
2.19.2

[Buildroot] [PATCHv2 5/8] support/download/file: implement source-check

[Thomas De Schampheleire]

From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>

Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
---
 support/download/file | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/support/download/file b/support/download/file
index e52fcf2c8c..bf3c428cbe 100755
--- a/support/download/file
+++ b/support/download/file
@@ -7,6 +7,7 @@ set -e
 #
 # Options:
 #   -q          Be quiet.
+#   -C          Only check that the source file exists.
 #   -o FILE     Copy to file FILE.
 #   -f FILE     Copy from basename file FILE.
 #   -u DIR      Copy from FILE in DIR.
@@ -23,6 +24,7 @@ verbose=-v
 while getopts "${BR_BACKEND_DL_GETOPTS}" OPT; do
     case "${OPT}" in
     q)  verbose=;;
+    C)  checkonly=1;;
     o)  output="${OPTARG}";;
     f)  file="${OPTARG}";;
     u)  dir="${OPTARG}";;
@@ -39,4 +41,12 @@ _localfiles() {
     eval ${LOCALFILES} "${@}"
 }
 
-_localfiles ${verbose} "'${dir##file://}/${file}'" "'${output}'"
+# Remove any scheme prefix
+dir="${dir##file://}"
+
+if [ -n "${checkonly}" ]; then
+    test -e "'${dir}/${file}'"
+    exit ${?}
+fi
+
+_localfiles ${verbose} "'${dir}/${file}'" "'${output}'"
-- 
2.19.2

[Buildroot] [PATCHv2 6/8] Config.in: reintroduce BR2_SSH

[Thomas De Schampheleire]

From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>

The BR2_SSH command was removed in commit
db9473bf6cd7bd12aa1f9faad0a917c973c33827 ("core/download: drop the SSH
command") but will be needed again to support 'source-check' for the scp
download backend.

Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
---
 Config.in               | 4 ++++
 package/pkg-download.mk | 1 +
 2 files changed, 5 insertions(+)

diff --git a/Config.in b/Config.in
index f965e9d6d8..03e4eb3928 100644
--- a/Config.in
+++ b/Config.in
@@ -136,6 +136,10 @@ config BR2_SCP
 	string "Secure copy (scp) command"
 	default "scp"
 
+config BR2_SSH
+	string "Secure shell (ssh) command"
+	default "ssh"
+
 config BR2_HG
 	string "Mercurial (hg) command"
 	default "hg"
diff --git a/package/pkg-download.mk b/package/pkg-download.mk
index cc04e316e2..e9506ae2a5 100644
--- a/package/pkg-download.mk
+++ b/package/pkg-download.mk
@@ -15,6 +15,7 @@ export BZR := $(call qstrip,$(BR2_BZR))
 export GIT := $(call qstrip,$(BR2_GIT))
 export HG := $(call qstrip,$(BR2_HG))
 export SCP := $(call qstrip,$(BR2_SCP))
+export SSH := $(call qstrip,$(BR2_SSH))
 export LOCALFILES := $(call qstrip,$(BR2_LOCALFILES))
 
 DL_WRAPPER = support/download/dl-wrapper
-- 
2.19.2

[Buildroot] [PATCHv2 7/8] support/download/scp: implement source-check

[Thomas De Schampheleire]

From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>

Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
---
 support/download/scp | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/support/download/scp b/support/download/scp
index 80cf495c4e..d81952956c 100755
--- a/support/download/scp
+++ b/support/download/scp
@@ -7,17 +7,20 @@ set -e
 #
 # Options:
 #   -q          Be quiet.
+#   -C          Only check that the file exists remotely.
 #   -o FILE     Copy to local file FILE.
 #   -f FILE     Copy from remote file FILE.
 #   -u URI      Download file at URI.
 #
 # Environment:
 #   SCP       : the scp command to call
+#   SSH       : the ssh command to use for checkonly
 
 verbose=
 while getopts "${BR_BACKEND_DL_GETOPTS}" OPT; do
     case "${OPT}" in
     q)  verbose=-q;;
+    C)  checkonly=1;;
     o)  output="${OPTARG}";;
     f)  filename="${OPTARG}";;
     u)  uri="${OPTARG}";;
@@ -33,8 +36,19 @@ shift $((OPTIND-1)) # Get rid of our options
 _scp() {
     eval ${SCP} "${@}"
 }
+_ssh() {
+    eval ${SSH} "${@}"
+}
 
 # Remove any scheme prefix
 uri="${uri##scp://}"
 
+if [ -n "${checkonly}" ]; then
+    # uri now looks like:  foo.example.org:some/directory
+    domain="${uri%%:*}"
+    path="${uri#*:}/${filename}"
+    _ssh ${verbose} "${@}" "'${domain}'" ls "'${path}'" > /dev/null
+    exit ${?}
+fi
+
 _scp ${verbose} "${@}" "'${uri}/${filename}'" "'${output}'"
-- 
2.19.2

[Buildroot] [PATCHv2 8/8] support/download/{bzr, cvs, git, svn}: highlight unimplemented source-check

[Thomas De Schampheleire]

From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>

For bzr, cvs, git and svn, there is no method of checking whether the
revision exists remotely, without actually cloning the repository.
Therefore, there is no actual implementation for '-C' (checkonly) for these
download helpers. The script will fall back to the normal download logic,
which would effectively fail if the revision does not exist.

For completeness, mention this unimplemented feature in the usage text.

Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
---
 support/download/bzr | 2 ++
 support/download/cvs | 2 ++
 support/download/git | 2 ++
 support/download/svn | 2 ++
 4 files changed, 8 insertions(+)

diff --git a/support/download/bzr b/support/download/bzr
index 5289a421cd..278d8778af 100755
--- a/support/download/bzr
+++ b/support/download/bzr
@@ -7,6 +7,8 @@ set -e
 #
 # Options:
 #   -q          Be quiet
+#   -C          (unimplemented) Only check that the revision exists in the
+#               remote repository.
 #   -o FILE     Generate archive in FILE.
 #   -u URI      Clone from repository at URI.
 #   -c CSET     Use changeset (or revision) CSET.
diff --git a/support/download/cvs b/support/download/cvs
index 9d0dc3cb3a..d9a586293a 100755
--- a/support/download/cvs
+++ b/support/download/cvs
@@ -7,6 +7,8 @@ set -e
 #
 # Options:
 #   -q          Be quiet
+#   -C          (unimplemented) Only check that the revision exists in the
+#               remote repository.
 #   -o FILE     Generate archive in FILE.
 #   -u URI      Checkout from repository at URI.
 #   -c REV      Use revision REV.
diff --git a/support/download/git b/support/download/git
index 17ca04eb98..e9c02d6712 100755
--- a/support/download/git
+++ b/support/download/git
@@ -7,6 +7,8 @@ set -E
 #
 # Options:
 #   -q          Be quiet.
+#   -C          (unimplemented) Only check that the revision exists in the
+#               remote repository.
 #   -r          Clone and archive sub-modules.
 #   -o FILE     Generate archive in FILE.
 #   -u URI      Clone from repository at URI.
diff --git a/support/download/svn b/support/download/svn
index 542b25c0a2..5fff064683 100755
--- a/support/download/svn
+++ b/support/download/svn
@@ -7,6 +7,8 @@ set -e
 #
 # Options:
 #   -q          Be quiet.
+#   -C          (unimplemented) Only check that the revision exists in the
+#               remote repository.
 #   -o FILE     Generate archive in FILE.
 #   -u URI      Checkout from repository at URI.
 #   -c REV      Use revision REV.
-- 
2.19.2

[Buildroot] [PATCHv2 0/8] fix scp and reintroduce source-check target

[Thomas Petazzoni]

On Mon,  4 Feb 2019 19:05:45 +0100
Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote:

> After the overwhelming support for 'source-check' at the Buildroot Developer
> Days ;-) , here is a second iteration of this series.

 :-)

> The first commit is actually a bugfix separate from source-check. In v1 of this
> series it was also included as the second of two 'scp'-related bugfixes, but it
> seems only the first bug-fix was applied at that time, which I hadn't realized
> until now.
> 
> Patches 2-8 are the actual source-check releated ones.
> For git, svn, bzr and cvs, there is no actual implementation which means that
> the corresponding download scripts will perform the standard download logic,
> which will also verify the validity of the specified revision on the remote
> server, but not as optimal as for the other methods.

What are the changes from v1 ? I don't see a changelog here or in the
patches themselves.

Perhaps you were overwhelmingly impatient to send this v2 that you
forgot the changelog ? :-)

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCHv2 0/8] fix scp and reintroduce source-check target

[Thomas De Schampheleire]

El lun., 4 feb. 2019 a las 19:24, Thomas Petazzoni
(<thomas.petazzoni@bootlin.com>) escribi?:
>
> On Mon,  4 Feb 2019 19:05:45 +0100
> Thomas De Schampheleire <patrickdepinguin@gmail.com> wrote:
>
> > After the overwhelming support for 'source-check' at the Buildroot Developer
> > Days ;-) , here is a second iteration of this series.
>
>  :-)
>
> > The first commit is actually a bugfix separate from source-check. In v1 of this
> > series it was also included as the second of two 'scp'-related bugfixes, but it
> > seems only the first bug-fix was applied at that time, which I hadn't realized
> > until now.
> >
> > Patches 2-8 are the actual source-check releated ones.
> > For git, svn, bzr and cvs, there is no actual implementation which means that
> > the corresponding download scripts will perform the standard download logic,
> > which will also verify the validity of the specified revision on the remote
> > server, but not as optimal as for the other methods.
>
> What are the changes from v1 ? I don't see a changelog here or in the
> patches themselves.
>
> Perhaps you were overwhelmingly impatient to send this v2 that you
> forgot the changelog ? :-)

Haha! You guessed it :-)

Changes from v2:
- add a trailing dot on all usage texts like for other options
- split off the reintroduction of BR2_SSH to a separate patch
- fix the source-check for scp: it seems there were some broken things in v1
- drop incomplete implementations for git/svn/bzr/cvs so it falls back
to actual download.

Best regards,
Thomas

[Buildroot] [PATCHv2 1/8] support/download/scp: fix download with scheme prefix 'scp://'

[Peter Korsgaard]

>>>>> "Thomas" == Thomas De Schampheleire <patrickdepinguin@gmail.com> writes:

 > From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
 > The scp download helper is broken when the server URL starts with 'scp://'.
 > Such prefix is used in two situations:
 > 1. to let FOO_SITE point to an scp location without explicitly having to set
 >    'FOO_SITE_METHOD = scp'

 > 2. when BR2_PRIMARY_SITE or BR2_BACKUP_SITE points to an scp location. In
 >    this case, there is no equivalent of 'SITE_METHOD'.

 > Strip out the scheme prefix, similarly to how the 'file' download helper
 > does it. That helper has the same cases as above.

 > Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>

Committed, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCHv2 2/8] support/download: reintroduce 'source-check' target

[Arnout Vandecappelle]

 Hi Thomas,

 As mentioned in the developer meeting, I agree that your use case is important
enough to bring source-check back. I do still have some feedback on the
implementation.

On 04/02/2019 19:05, Thomas De Schampheleire wrote:
[snip]
> +define SOURCE_CHECK
> +	$(Q)mkdir -p $($(PKG)_DL_DIR)
> +	$(Q)$(EXTRA_ENV) $(FLOCK) $(DL_WRAPPER) \
> +		-C \

 Except for the -C, this is identical to the DOWNLOAD macro and should stay that
way. So I'd propose to add a second argument to DOWNLOAD in which you can pass -C.

> +		-c '$($(PKG)_DL_VERSION)' \
> +		-d '$($(PKG)_DL_DIR)' \
> +		-D '$(DL_DIR)' \
> +		-f '$(notdir $(1))' \
> +		-H '$($(PKG)_HASH_FILE)' \
> +		-n '$($(PKG)_BASENAME_RAW)' \
> +		-N '$($(PKG)_RAWNAME)' \
> +		-o '$($(PKG)_DL_DIR)/$(notdir $(1))' \
> +		$(if $($(PKG)_GIT_SUBMODULES),-r) \
> +		$(DOWNLOAD_URIS) \
> +		$(QUIET) \
> +		-- \
> +		$($(PKG)_DL_OPTS)
> +endef
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index f5cab2b9c2..707996e384 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -780,6 +780,10 @@ $(1)-legal-source:	$$($(2)_TARGET_ACTUAL_SOURCE)
>  endif # actual sources != sources
>  endif # actual sources != ""
>  
> +$(1)-source-check: PKG=$(2)
> +$(1)-source-check:
> +	$$(foreach p,$$($(2)_ALL_DOWNLOADS),$$(call SOURCE_CHECK,$$(p))$$(sep))

 Hm, I wonder if the pre/post download hooks shouldn't be called as well... We
don't have any in-tree consumers of them, so it's hard to be sure.

[snip]
>  
>      # Look through all the uris that we were given to download the package
> @@ -127,7 +130,7 @@ main() {
>                  -f "${filename}" \
>                  -u "${uri}" \
>                  -o "${tmpf}" \
> -                ${quiet} ${recurse} -- "${@}"
> +                ${quiet} ${recurse} ${checkonly} -- "${@}"
>          then
>              # cd back to keep path coherence
>              cd "${OLDPWD}"

 Hm, I wonder if this is the correct semantics for source-check. It will check
if the package can be found in any of the URLs, but in practice, don't you want
to to be in the first one only? For the primary case, yhou'll pass PRIMARY_ONLY
of course; but if you use it to check if the upstream URL is still correct, then
you want it to fail if you can't get it there. Of course, you can do that by
clearing the SECONDARY. But is there ever any reason to check if the package
exists at any of the URLs?

 That said, special-casing the checkonly here again will only make the control
flow even more complicated...

> @@ -138,19 +141,21 @@ main() {
>          # cd back to free the temp-dir, so we can remove it later
>          cd "${OLDPWD}"
>  
> -        # Check if the downloaded file is sane, and matches the stored hashes
> -        # for that file
> -        if support/download/check-hash ${quiet} "${hfile}" "${tmpf}" "${output##*/}"; then
> -            rc=0
> -        else
> -            if [ ${?} -ne 3 ]; then
> -                rm -rf "${tmpd}"
> -                continue
> +        if [ -z "${checkonly}" ]; then
> +            # Check if the downloaded file is sane, and matches the stored hashes
> +            # for that file
> +            if support/download/check-hash ${quiet} "${hfile}" "${tmpf}" "${output##*/}"; then
> +                rc=0
> +            else
> +                if [ ${?} -ne 3 ]; then
> +                    rm -rf "${tmpd}"
> +                    continue
> +                fi
> +
> +                # the hash file exists and there was no hash to check the file
> +                # against
> +                rc=1
>              fi
> -
> -            # the hash file exists and there was no hash to check the file
> -            # against
> -            rc=1
>          fi
>          download_and_check=1
>          break
> @@ -163,6 +168,12 @@ main() {
>          exit 1
>      fi
>  
> +    # If we only need to check the presence of sources, stop here.
> +    # No need to handle output files.

 So, without the subsequent patches, it actually *will* have downloaded
something (and even with the subsequent patches it will have done that for
git/svn/...). So shouldn't the downloaded file be removed again? I'm not sure
though...

 Regards,
 Arnout

> +    if [ -n "${checkonly}" ]; then
> +        exit 0
> +    fi
> +
>      # tmp_output is in the same directory as the final output, so we can
>      # later move it atomically.
>      tmp_output="$(mktemp "${output}.XXXXXX")"
> 

[Buildroot] [PATCHv2 7/8] support/download/scp: implement source-check

[Arnout Vandecappelle]

On 04/02/2019 19:05, Thomas De Schampheleire wrote:
> +if [ -n "${checkonly}" ]; then
> +    # uri now looks like:  foo.example.org:some/directory
> +    domain="${uri%%:*}"
> +    path="${uri#*:}/${filename}"
> +    _ssh ${verbose} "${@}" "'${domain}'" ls "'${path}'" > /dev/null

 I don't know if it happens in practice, but it is possible to have scp access
to a machine but no 'ls' that you can run over ssh.

 Regards,
 Arnout

> +    exit ${?}
> +fi

[Buildroot] [PATCHv2 2/8] support/download: reintroduce 'source-check' target

[Thomas De Schampheleire]

El s?b., 9 feb. 2019 a las 17:47, Arnout Vandecappelle
(<arnout@mind.be>) escribi?:
>
>  Hi Thomas,
>
>  As mentioned in the developer meeting, I agree that your use case is important
> enough to bring source-check back. I do still have some feedback on the
> implementation.
>
> On 04/02/2019 19:05, Thomas De Schampheleire wrote:
> [snip]
> > +define SOURCE_CHECK
> > +     $(Q)mkdir -p $($(PKG)_DL_DIR)
> > +     $(Q)$(EXTRA_ENV) $(FLOCK) $(DL_WRAPPER) \
> > +             -C \
>
>  Except for the -C, this is identical to the DOWNLOAD macro and should stay that
> way. So I'd propose to add a second argument to DOWNLOAD in which you can pass -C.

Ok, will do.
I will keep a separate SOURCE_CHECK which forwards directly to
DOWNLOAD with extra argument, for clarity for callers.

>
> > +             -c '$($(PKG)_DL_VERSION)' \
> > +             -d '$($(PKG)_DL_DIR)' \
> > +             -D '$(DL_DIR)' \
> > +             -f '$(notdir $(1))' \
> > +             -H '$($(PKG)_HASH_FILE)' \
> > +             -n '$($(PKG)_BASENAME_RAW)' \
> > +             -N '$($(PKG)_RAWNAME)' \
> > +             -o '$($(PKG)_DL_DIR)/$(notdir $(1))' \
> > +             $(if $($(PKG)_GIT_SUBMODULES),-r) \
> > +             $(DOWNLOAD_URIS) \
> > +             $(QUIET) \
> > +             -- \
> > +             $($(PKG)_DL_OPTS)
> > +endef
> > diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> > index f5cab2b9c2..707996e384 100644
> > --- a/package/pkg-generic.mk
> > +++ b/package/pkg-generic.mk
> > @@ -780,6 +780,10 @@ $(1)-legal-source:       $$($(2)_TARGET_ACTUAL_SOURCE)
> >  endif # actual sources != sources
> >  endif # actual sources != ""
> >
> > +$(1)-source-check: PKG=$(2)
> > +$(1)-source-check:
> > +     $$(foreach p,$$($(2)_ALL_DOWNLOADS),$$(call SOURCE_CHECK,$$(p))$$(sep))
>
>  Hm, I wonder if the pre/post download hooks shouldn't be called as well... We
> don't have any in-tree consumers of them, so it's hard to be sure.

Yes, it could go both ways.
Hooks could assume there is an actual download and behave that way.
But in theory they could also be required to, say, open up a firewall
port, and thus also be needed for source-check.

I tend to think that any 'DOWNLOAD_HOOK' would actually expect
downloads. Also because until now source-check was gone.
If someone comes up with a source-check-hook use case, we would need
to see whether it should be inside DOWNLOAD_HOOKs (which may need
differentiation between downloads and source-checks) or in a separate
SOURCE_CHECK_HOOK.

>
> [snip]
> >
> >      # Look through all the uris that we were given to download the package
> > @@ -127,7 +130,7 @@ main() {
> >                  -f "${filename}" \
> >                  -u "${uri}" \
> >                  -o "${tmpf}" \
> > -                ${quiet} ${recurse} -- "${@}"
> > +                ${quiet} ${recurse} ${checkonly} -- "${@}"
> >          then
> >              # cd back to keep path coherence
> >              cd "${OLDPWD}"
>
>  Hm, I wonder if this is the correct semantics for source-check. It will check
> if the package can be found in any of the URLs, but in practice, don't you want
> to to be in the first one only? For the primary case, yhou'll pass PRIMARY_ONLY
> of course; but if you use it to check if the upstream URL is still correct, then
> you want it to fail if you can't get it there. Of course, you can do that by
> clearing the SECONDARY. But is there ever any reason to check if the package
> exists at any of the URLs?
>
>  That said, special-casing the checkonly here again will only make the control
> flow even more complicated...

dl-wrapper just gets a list of URIs to try, without metadata about the
type of URI this is. So knowledge about primary-site, backup-site,
etc. is not known here.

Even if you're going for the primary site, assuming that you'd need
the first URL only is not correct: the download is first attempted
from a per-package subdir, e.g. 'busybox/busybox-1.29.tar.gz', and
then falls back to the old flat scheme 'busybox-1.29.tar.gz'. This is
an implementation detail that the dl-wrapper should not rely on IMHO.

So yes, the user of source-check needs to decide what exactly they
want to check and set/clear BR2_PRIMARY_SITE, BR2_PRIMARY_SITE_ONLY,
and BR2_BACKUP_SITE accordingly.

>
> > @@ -138,19 +141,21 @@ main() {
> >          # cd back to free the temp-dir, so we can remove it later
> >          cd "${OLDPWD}"
> >
> > -        # Check if the downloaded file is sane, and matches the stored hashes
> > -        # for that file
> > -        if support/download/check-hash ${quiet} "${hfile}" "${tmpf}" "${output##*/}"; then
> > -            rc=0
> > -        else
> > -            if [ ${?} -ne 3 ]; then
> > -                rm -rf "${tmpd}"
> > -                continue
> > +        if [ -z "${checkonly}" ]; then
> > +            # Check if the downloaded file is sane, and matches the stored hashes
> > +            # for that file
> > +            if support/download/check-hash ${quiet} "${hfile}" "${tmpf}" "${output##*/}"; then
> > +                rc=0
> > +            else
> > +                if [ ${?} -ne 3 ]; then
> > +                    rm -rf "${tmpd}"
> > +                    continue
> > +                fi
> > +
> > +                # the hash file exists and there was no hash to check the file
> > +                # against
> > +                rc=1
> >              fi
> > -
> > -            # the hash file exists and there was no hash to check the file
> > -            # against
> > -            rc=1
> >          fi
> >          download_and_check=1
> >          break
> > @@ -163,6 +168,12 @@ main() {
> >          exit 1
> >      fi
> >
> > +    # If we only need to check the presence of sources, stop here.
> > +    # No need to handle output files.
>
>  So, without the subsequent patches, it actually *will* have downloaded
> something (and even with the subsequent patches it will have done that for
> git/svn/...). So shouldn't the downloaded file be removed again? I'm not sure
> though...

Yes, indeed. There should be a 'rm -rf ${tmpd}' here.

Thanks for the feedback,
Thomas

[Buildroot] [PATCHv2 7/8] support/download/scp: implement source-check

[Thomas De Schampheleire]

El s?b., 9 feb. 2019 a las 17:48, Arnout Vandecappelle
(<arnout@mind.be>) escribi?:
>
>
>
> On 04/02/2019 19:05, Thomas De Schampheleire wrote:
> > +if [ -n "${checkonly}" ]; then
> > +    # uri now looks like:  foo.example.org:some/directory
> > +    domain="${uri%%:*}"
> > +    path="${uri#*:}/${filename}"
> > +    _ssh ${verbose} "${@}" "'${domain}'" ls "'${path}'" > /dev/null
>
>  I don't know if it happens in practice, but it is possible to have scp access
> to a machine but no 'ls' that you can run over ssh.

Yes, that is true.
If we could know that the 'ssh ls' fails because it is not supported,
we could fall back to a real scp download. But I don't think we can
differentiate that case from the case where ls works but the file is
simply not present.

So the only thing I can come up with is keep this 'ssh ls'.

One could reason that 'ssh/scp' already needs some semi-intimate
relation between the user and the server due to the trusting of the
keypair, so that it is more likely that 'ssh ls' is available, and if
it is not then user and server admin could work together to fix that.

Best regards,
Thomas

[Buildroot] [PATCHv2 1/8] support/download/scp: fix download with scheme prefix 'scp://'

[Peter Korsgaard]

>>>>> "Thomas" == Thomas De Schampheleire <patrickdepinguin@gmail.com> writes:

 > From: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
 > The scp download helper is broken when the server URL starts with 'scp://'.
 > Such prefix is used in two situations:
 > 1. to let FOO_SITE point to an scp location without explicitly having to set
 >    'FOO_SITE_METHOD = scp'

 > 2. when BR2_PRIMARY_SITE or BR2_BACKUP_SITE points to an scp location. In
 >    this case, there is no equivalent of 'SITE_METHOD'.

 > Strip out the scheme prefix, similarly to how the 'file' download helper
 > does it. That helper has the same cases as above.

 > Signed-off-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>

Committed to 2018.11.x, thanks.

-- 
Bye, Peter Korsgaard