Skip to contentSkip to conversation

support/download: be more aggressive on missing hashes (branch yem/dl-hash-2)

PATCH Superseded Yann E. MORIN 3 participants 19 messages Mar 31, 2015 – Apr 3, 2015

lore
Share & tools
lore
#0 lore raw

v4 cover support/download: be more aggressive on missing hashes (branch yem/dl-hash-2)

Hello All!

This series makes hashes mandatory when a .hash file exists.

Currently, we treat missing hashes as a mere warning. Unfortunately, that often goes un-noticed by most users, and thus we get a lot of package bumps that are missing the new hashes corresponding to the new version.

We now make that a hard error, so users really notice something is wrong.

Of course, if no .hash file exists, the behaviour is as yet unchanged.

Changes v3 -> v4:
- fix typoes (Samuel)

Changes v2 -> v3:
- further differentiat hash check errors (Arnout)
- add 'none' hash type (Arnout)
- further improve the manual (Arnout)

Changes v1 -> v2:
- make it work for downloads from git/svn/... repositories

Regards,
Yann E. MORIN.

The following changes since commit f3383b0eabab16865c630710f71d318bcc9e9f38:

python-ws4py: new package (2015-04-01 00:05:15 +0200)

are available in the git repository at:

git://git.busybox.net/~ymorin/git/buildroot yem/dl-hash-2

for you to fetch changes up to 4a4bae18e06b5e4205eb64c1384485c920325aa3:

support/download: warn when there's no .hash file (2015-04-01 00:11:28 +0200)

----------------------------------------------------------------
Yann E. MORIN (7):
support/download: make hash file optional
package infra: do not check hashes when downloading from a repository support/download: add explicit no-hash support support/download: return different exit codes for different failures support/download: properly catch missing hashes support/download: always fail when there's no hash support/download: warn when there's no .hash file

 docs/manual/adding-packages-directory.txt | 27 +++++++++++++++++++++------
 package/pkg-download.mk                   |  5 -----
 support/download/check-hash               | 28 +++++++++++++++++++---------
 support/download/dl-wrapper               | 14 ++++++++++----
 4 files changed, 50 insertions(+), 24 deletions(-)
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------.
|  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.  |
'------------------------------^-------^------------------^--------------------'
#1 lore raw

PATCH v4 1/7 support/download: make hash file optional

Currently, specifying a hash file for our download wrapper is mandatory.

However, when we download a git, svn, bzr, hg or cvs tree, there's by design no hash to check the download against.

Since we're going to have hash checking mandatory when a hash file exists, this would break those downloads from a repository.

So, make specifying a hash file optional when calling our download wrapper and bail out early from the check-hash script if no hash file is specified.

---
 support/download/check-hash | 2 +-
 support/download/dl-wrapper | 3 ---
 2 files changed, 1 insertion(+), 4 deletions(-)
support/download/check-hash +1 -1
index 4c07274..cee64ef 100755
@@ -23,7 +23,7 @@ file="${2}"
 base="${3}"
 
 # Does the hash-file exist?
-if [ ! -f "${h_file}" ]; then
+if [ -z "${h_file}" -o ! -f "${h_file}" ]; then
     exit 0
 fi
 
support/download/dl-wrapper +0 -3
index 3b30840..514118c 100755
@@ -44,9 +44,6 @@ main() {
     if [ -z "${output}" ]; then
         error "no output specified, use -o\n"
     fi
-    if [ -z "${hfile}" ]; then
-        error "no hash-file specified, use -H\n"
-    fi
 
     # If the output file already exists, do not download it again
     if [ -e "${output}" ]; then
Signature · 1 line
1.9.1
#2 lore raw

PATCH v4 2/7 package infra: do not check hashes when downloading from a repository

In reply to Yann E. MORIN

When downloading from a repository, we have no way to ensure the reproducibility of the generated archives, so we can't check the hashes.

Do not specifiy a hash file in those cases.

---
Changes v2 -> v3:
- add blurb in the manual (Arnout)

---
 docs/manual/adding-packages-directory.txt | 6 ++++++
 package/pkg-download.mk                   | 5 -----
 2 files changed, 6 insertions(+), 5 deletions(-)
docs/manual/adding-packages-directory.txt +6 -0
index 1ce9a3b..132c702 100644
@@ -474,4 +474,10 @@ environment variable +BR2_ENFORCE_CHECK_HASH+ to a non-empty value, and
 there is no hash for a downloaded file, Buildroot considers this an
 error, deletes the downloaded file, and aborts.
 
+Sources that are downloaded from a version control system (git, subversion,
+etc...) can not have a hash, because the version control system and tar
+may not create exactly the same file (dates, files ordering...), so the
+hash could be wrong even for a valid download. Therefore, the hash check
+is entirely skipped for such sources.
+
 If the +.hash+ file is missing, then no check is done at all.
package/pkg-download.mk +0 -5
index 5e74519..e274712 100644
@@ -87,7 +87,6 @@ github = https://github.com/$(1)/$(2)/archive/$(3)
 define DOWNLOAD_GIT
 	$(EXTRA_ENV) $(DL_WRAPPER) -b git \
 		-o $(DL_DIR)/$($(PKG)_SOURCE) \
-		-H $(PKGDIR)/$($(PKG)_RAWNAME).hash \
 		$(QUIET) \
 		-- \
 		$($(PKG)_SITE) \
@@ -109,7 +108,6 @@ endef
 define DOWNLOAD_BZR
 	$(EXTRA_ENV) $(DL_WRAPPER) -b bzr \
 		-o $(DL_DIR)/$($(PKG)_SOURCE) \
-		-H $(PKGDIR)/$($(PKG)_RAWNAME).hash \
 		$(QUIET) \
 		-- \
 		$($(PKG)_SITE) \
@@ -128,7 +126,6 @@ endef
 define DOWNLOAD_CVS
 	$(EXTRA_ENV) $(DL_WRAPPER) -b cvs \
 		-o $(DL_DIR)/$($(PKG)_SOURCE) \
-		-H $(PKGDIR)/$($(PKG)_RAWNAME).hash \
 		$(QUIET) \
 		-- \
 		$(call stripurischeme,$(call qstrip,$($(PKG)_SITE))) \
@@ -149,7 +146,6 @@ endef
 define DOWNLOAD_SVN
 	$(EXTRA_ENV) $(DL_WRAPPER) -b svn \
 		-o $(DL_DIR)/$($(PKG)_SOURCE) \
-		-H $(PKGDIR)/$($(PKG)_RAWNAME).hash \
 		$(QUIET) \
 		-- \
 		$($(PKG)_SITE) \
@@ -189,7 +185,6 @@ endef
 define DOWNLOAD_HG
 	$(EXTRA_ENV) $(DL_WRAPPER) -b hg \
 		-o $(DL_DIR)/$($(PKG)_SOURCE) \
-		-H $(PKGDIR)/$($(PKG)_RAWNAME).hash \
 		$(QUIET) \
 		-- \
 		$($(PKG)_SITE) \
Signature · 1 line
1.9.1
#3 lore raw

PATCH v4 2/7 package infra: do not check hashes when downloading from a repository

Quoted 8 lines · Yann E. MORIN

When downloading from a repository, we have no way to ensure the reproducibility of the generated archives, so we can't check the hashes.

Do not specifiy a hash file in those cases.

And now I also reviewed the manual update.

Regards,
Arnout

Quoted 5 lines

---
Changes v2 -> v3:
- add blurb in the manual (Arnout)

[snip]

Signature · 6 lines
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F
#4 lore raw

PATCH v4 2/7 package infra: do not check hashes when downloading from a repository

Arnout, All,

On 2015-04-02 23:38 +0200, Arnout Vandecappelle spake thusly:

Quoted 11 lines · Yann E. MORIN
Quoted 8 lines

When downloading from a repository, we have no way to ensure the reproducibility of the generated archives, so we can't check the hashes.

Do not specifiy a hash file in those cases.

And now I also reviewed the manual update.

Well, I think I just used what you provided in your previous review, so I assumed your reviewed-by tag was still valid... ;-)

Thanks!

Regards,
Yann E. MORIN.

Signature · 6 lines
.-----------------.--------------------.------------------.--------------------.
|  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.  |
'------------------------------^-------^------------------^--------------------'
#5 lore raw

PATCH v4 2/7 package infra: do not check hashes when downloading from a repository

Quoted 9 lines · Yann E. MORIN

Arnout, All,

On 2015-04-02 23:38 +0200, Arnout Vandecappelle spake thusly:

Quoted 6 lines · Yann E. MORIN
Quoted 5 lines

When downloading from a repository, we have no way to ensure the reproducibility of the generated archives, so we can't check the hashes.

Do not specifiy a hash file in those cases.

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

And now I also reviewed the manual update.

Well, I think I just used what you provided in your previous review, so I assumed your reviewed-by tag was still valid... ;-)

Ah, that explains the complete lack of spelling mistakes :-P

Regards,
Arnout

Signature · 6 lines
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F
#6 lore raw

PATCH v4 2/7 package infra: do not check hashes when downloading from a repository

Arnout, All,

On 2015-04-02 23:57 +0200, Arnout Vandecappelle spake thusly:

Quoted 1 line · Yann E. MORIN

[---SNIP--]

Quoted 4 lines
Quoted 2 lines

Well, I think I just used what you provided in your previous review, so I assumed your reviewed-by tag was still valid... ;-)

Ah, that explains the complete lack of spelling mistakes :-P

You now *are* on my List. Beware...

Regards,
Yann E. MORIN.

PS. Muhahaha! ;-)

Signature · 6 lines
.-----------------.--------------------.------------------.--------------------.
|  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.  |
'------------------------------^-------^------------------^--------------------'
#7 lore raw

PATCH v4 3/7 support/download: add explicit no-hash support

In reply to Yann E. MORIN

Add support to explicitly state that an archive has no hash.

This can be used for archives downloaded from a repository, like a git-clone or a subversion checkout, or using the github helper.

This will come in handy when we'll eventually make hashes mandatory as soon as a .hash file exists: for some packages, like gcc, some versions are downloaded as archives from upstream, while other versions may come from a GitHub repository (via the github herlper).

In this case, a .hash file would exist, that contains hashes for the downloaded tarballs, but archives downloaded from the repository would not have a hash (since it is currently not possible to have reproducible such archives). So, we'd need a way to explicitly state there is no hash, on purpose, for those archives.

So, add 'none' as a new type of hash.

---
Changes v3 -> v4:
- code formatting (Arnout)

---
 docs/manual/adding-packages-directory.txt | 13 +++++++++++--
 support/download/check-hash               |  4 ++++
 2 files changed, 15 insertions(+), 2 deletions(-)
docs/manual/adding-packages-directory.txt +11 -2
index 132c702..0f8bb64 100644
@@ -423,8 +423,9 @@ The format of this file is one line for each file for which to check the
 hash, each line being space-separated, with these three fields:
 
 * the type of hash, one of:
-** +sha1+, +sha224+, +sha256+, +sha384+, +sha512+
+** +sha1+, +sha224+, +sha256+, +sha384+, +sha512+, +none+
 * the hash of the file:
+** for +none+, one or more non-space chars, usually just the string +xxx+
 ** for +sha1+, 40 hexadecimal characters
 ** for +sha224+, 56 hexadecimal characters
 ** for +sha256+, 64 hexadecimal characters
@@ -448,9 +449,14 @@ comment line above the hashes.
 *Note:* the number of spaces does not matter, so one can use spaces to
 properly align the different fields.
 
+The +none+ hash type is reserved to those archives downloaded from a
+repository, like a 'git clone', a 'subversion checkout'... or archives
+downloaded with the xref:github-download-url[github helper].
+
 The example below defines a +sha1+ and a +sha256+ published by upstream for
 the main +libfoo-1.2.3.tar.bz2+ tarball, plus two locally-computed hashes,
-a +sha256+ for a downloaded patch, and a +sha1+ for a downloaded binary blob:
+a +sha256+ for a downloaded patch, a +sha1+ for a downloaded binary blob,
+and an archive with no hash:
 
 ----
 # Hashes from: http://www.foosoftware.org/download/libfoo-1.2.3.tar.bz2.{sha1,sha256}:
@@ -460,6 +466,9 @@ sha256 efc8103cc3bcb06bda6a781532d12701eb081ad83e8f90004b39ab81b65d4369 libfoo-1
 # No upstream hashes for the following:
 sha256 ff52101fb90bbfc3fe9475e425688c660f46216d7e751c4bbdb1dc85cdccacb9 libfoo-fix-blabla.patch
 sha1   2d608f3c318c6b7557d551a5a09314f03452f1a1                         libfoo-data.bin
+
+# Explicitly no hash for that file, comes from a git-clone:
+none   xxx                                                              libfoo-1234.tar.gz
 ----
 
 If the +.hash+ file is present, and it contains one or more hashes for a
support/download/check-hash +4 -0
index cee64ef..7a30d5b 100755
@@ -38,7 +38,11 @@ check_one_hash() {
     # Note: md5 is supported, but undocumented on purpose.
     # Note: sha3 is not supported, since there is currently no implementation
     #       (the NIST has yet to publish the parameters).
+    # Note: 'none' means there is explicitly no hash for that file.
     case "${_h}" in
+        none)
+            return 0
+            ;;
         md5|sha1)                       ;;
         sha224|sha256|sha384|sha512)    ;;
         *) # Unknown hash, exit with error
Signature · 1 line
1.9.1
#8 lore raw

PATCH v4 4/7 support/download: return different exit codes for different failures

In reply to Yann E. MORIN

Return different exit codes depending on the error that occured:

0: no error (hash file missing, or all hashes match) 1: unknown option
2: hash file exists, but at least one hash in error 3: hash file exists, but no hash for file to check 4: hash file exists, but at least one hash type unknown

This will be used in a later patch to decide whether the downloaded file should be kept or removed.

Cc Thomas Petazzoni <[email protected]>
Cc Samuel Martin <[email protected]>
Cc Arnout Vandecappelle <[email protected]>

---
Changes v3 -> v4:
- typo in comment (Samuel)

Changes v2 -> v3:
- further differentiate errors for unknown hashes and unknown command-line options (Arnout)

Changes v1 -> v2:
- typoes in script and commit (Samuel)

---
 support/download/check-hash | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)
support/download/check-hash +12 -3
index 7a30d5b..ab416c7 100755
@@ -9,6 +9,15 @@ set -e
 #   $3: the final basename of the file, to which it will be ultimately
 #       saved as, to be able to match it to the corresponding hashes
 #       in the .hash file
+#
+# Exit codes:
+#   0:  the hash file exists and the file to check matches all its hashes,
+#       or the hash file does not exist
+#   1:  unknown command-line option
+#   2:  the hash file exists and the file to check does not match at least
+#       one of its hashes
+#   3:  the hash file exists and there was no hash to check the file against
+#   4:  the hash file exists and at least one hash type is unknown
 
 while getopts :q OPT; do
     case "${OPT}" in
@@ -48,7 +57,7 @@ check_one_hash() {
         *) # Unknown hash, exit with error
             printf "ERROR: unknown hash '%s' for '%s'\n"  \
                    "${_h}" "${base}" >&2
-            exit 1
+            exit 4
             ;;
     esac
 
@@ -64,7 +73,7 @@ check_one_hash() {
     printf "ERROR: got     : %s\n" "${_hash}" >&2
     printf "ERROR: Incomplete download, or man-in-the-middle (MITM) attack\n" >&2
 
-    exit 1
+    exit 2
 }
 
 # Do we know one or more hashes for that file?
@@ -87,7 +96,7 @@ done <"${h_file}"
 if [ ${nb_checks} -eq 0 ]; then
     if [ -n "${BR2_ENFORCE_CHECK_HASH}" ]; then
         printf "ERROR: No hash found for %s\n" "${base}" >&2
-        exit 1
+        exit 3
     else
         printf "WARNING: No hash found for %s\n" "${base}" >&2
     fi
Signature · 1 line
1.9.1
#9 lore raw

PATCH v4 4/7 support/download: return different exit codes for different failures

Quoted 8 lines · Yann E. MORIN

Return different exit codes depending on the error that occured:

0: no error (hash file missing, or all hashes match) 1: unknown option
2: hash file exists, but at least one hash in error 3: hash file exists, but no hash for file to check 4: hash file exists, but at least one hash type unknown

Quoted 7 lines

This will be used in a later patch to decide whether the downloaded file should be kept or removed.

Cc Thomas Petazzoni <[email protected]>
Cc Samuel Martin <[email protected]>
Cc Arnout Vandecappelle <[email protected]>

Regards,
Arnout

[snip]

Signature · 6 lines
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F
#10 lore raw

PATCH v4 5/7 support/download: properly catch missing hashes

In reply to Yann E. MORIN

When checking hashes reports no hash for a file, and this is treated as an error (now: because BR2_ENFORCE_CHECK_HASH is set; later: because that will be the new and only behaviour), exit promptly in error.

---
Changes v2 -> v3:
- change the exit code to 1 (no need to differentiate exit codes here)

---
 support/download/dl-wrapper | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)
support/download/dl-wrapper +10 -1
index 514118c..ef2d872 100755
@@ -45,10 +45,19 @@ main() {
         error "no output specified, use -o\n"
     fi
 
-    # If the output file already exists, do not download it again
+    # 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##*/}"
Signature · 1 line
1.9.1
#11 lore raw

PATCH v4 6/7 support/download: always fail when there's no hash

In reply to Yann E. MORIN

At the time we introduced hashes, we did not want to be too harsh in the beginning, and give people some time to adapt and accept the hashes. So we so far only whined^Wwarned about a missing hash (when the .hash file exists).

Some time has passed now, and people are still missing updating hashes when bumping packages.

Let's make that warning a little bit more annoying...

Cc Thomas Petazzoni <[email protected]>
Cc Arnout Vandecappelle <[email protected]>

---
Changes v2 -> v3:
- further improve the manual (Arnout)

---
 docs/manual/adding-packages-directory.txt | 8 ++++----
 support/download/check-hash               | 8 ++------
 2 files changed, 6 insertions(+), 10 deletions(-)
docs/manual/adding-packages-directory.txt +4 -4
index 0f8bb64..0051fea 100644
@@ -478,10 +478,10 @@ not match, Buildroot considers this an error, deletes the downloaded file,
 and aborts.
 
 If the +.hash+ file is present, but it does not contain a hash for a
-downloaded file, no check is done for that file. If you set the
-environment variable +BR2_ENFORCE_CHECK_HASH+ to a non-empty value, and
-there is no hash for a downloaded file, Buildroot considers this an
-error, deletes the downloaded file, and aborts.
+downloaded file, Buildroot considers this an error and aborts. However,
+the downloaded file is left in the download directory since this
+typically indicates that the +.hash+ file is wrong but the downloaded
+file is probably OK.
 
 Sources that are downloaded from a version control system (git, subversion,
 etc...) can not have a hash, because the version control system and tar
support/download/check-hash +2 -6
index ab416c7..13a76b8 100755
@@ -94,10 +94,6 @@ while read t h f; do
 done <"${h_file}"
 
 if [ ${nb_checks} -eq 0 ]; then
-    if [ -n "${BR2_ENFORCE_CHECK_HASH}" ]; then
-        printf "ERROR: No hash found for %s\n" "${base}" >&2
-        exit 3
-    else
-        printf "WARNING: No hash found for %s\n" "${base}" >&2
-    fi
+    printf "ERROR: No hash found for %s\n" "${base}" >&2
+    exit 3
 fi
Signature · 1 line
1.9.1
#12 lore raw

PATCH v4 7/7 support/download: warn when there's no .hash file

In reply to Yann E. MORIN

Instead of silently accepting a missing .hash file, print a warning.

This can be grepped from a build log, to find packages that still have no hash, with the long-term goal of adding hashes for all packages.

---
 support/download/check-hash | 1 +
 1 file changed, 1 insertion(+)
support/download/check-hash +1 -0
index 13a76b8..3483077 100755
@@ -33,6 +33,7 @@ base="${3}"
 
 # Does the hash-file exist?
 if [ -z "${h_file}" -o ! -f "${h_file}" ]; then
+    printf "WARNING: no hash file for %s\n" "${base}" >&2
     exit 0
 fi
 
Signature · 1 line
1.9.1
#13 lore raw

v4 cover support/download: be more aggressive on missing hashes (branch yem/dl-hash-2)

In reply to Yann E. MORIN

Dear Yann E. MORIN,

Quoted 9 lines · Yann E. MORIN

Hello All!

This series makes hashes mandatory when a .hash file exists.

Currently, we treat missing hashes as a mere warning. Unfortunately, that often goes un-noticed by most users, and thus we get a lot of package bumps that are missing the new hashes corresponding to the new version.

Quoted 4 lines

We now make that a hard error, so users really notice something is wrong.

Of course, if no .hash file exists, the behaviour is as yet unchanged.

I applied this and was going to push it, but I believe there's still a problem.

If I change strace.hash so that there is no hash matching the tarball name of strace, then I get two times the error:

$ make strace-extract
ERROR: No hash found for strace-4.10.tar.xz
ERROR: No hash found for strace-4.10.tar.xz
package/pkg-generic.mk:73: recipe for target '/home/thomas/projets/buildroot/output/build/strace-4.10/.stamp_downloaded' failed

I haven't looked too deeply, but I believe it's because check-hash returns 3, so dl-wrapper exits with error code 1, which means that the pkg-download.mk logic concludes that the download from the upstream location has failed, so it retries with sources.buildroot.net, and the same thing happens.

Is this expected?

Best regards,

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
#14 lore raw

v4 cover support/download: be more aggressive on missing hashes (branch yem/dl-hash-2)

Thomas, All,

On 2015-04-01 22:08 +0200, Thomas Petazzoni spake thusly:

Quoted 2 lines · Yann E. MORIN
Quoted 1 line

This series makes hashes mandatory when a .hash file exists.

[--SNIP--]

Quoted 11 lines

I applied this and was going to push it, but I believe there's still a problem.

If I change strace.hash so that there is no hash matching the tarball name of strace, then I get two times the error:

$ make strace-extract
ERROR: No hash found for strace-4.10.tar.xz
ERROR: No hash found for strace-4.10.tar.xz
package/pkg-generic.mk:73: recipe for target '/home/thomas/projets/buildroot/output/build/strace-4.10/.stamp_downloaded' failed
Quoted 7 lines

I haven't looked too deeply, but I believe it's because check-hash returns 3, so dl-wrapper exits with error code 1, which means that the pkg-download.mk logic concludes that the download from the upstream location has failed, so it retries with sources.buildroot.net, and the same thing happens.

Is this expected?

That's at least the result I expect, yes.

I know this might look weird, indeed. However, I did not find a simple
way to avoid this.

The problem is that we have a biggish block of succesive conditions in
package/pkg-download:

    define DOWNLOAD_INNER
        $(Q)if test -n "$(call qstrip,$(BR2_PRIMARY_SITE))" ; then \
                case "$(call geturischeme,$(BR2_PRIMARY_SITE))" in \
                        scp) $(call $(DL_MODE)_SCP,$(BR2_PRIMARY_SITE)/$(2),$(2)) && exit ;; \
                        *) $(call $(DL_MODE)_WGET,$(BR2_PRIMARY_SITE)/$(2),$(2)) && exit ;; \
                esac ; \
        fi ; \
        if test "$(BR2_PRIMARY_SITE_ONLY)" = "y" ; then \
                exit 1 ; \
        fi ; \
        if test -n "$(1)" ; then \
                if test -z "$($(PKG)_SITE_METHOD)" ; then \
                        scheme="$(call geturischeme,$(1))" ; \
                else \
                        scheme="$($(PKG)_SITE_METHOD)" ; \
                fi ; \
                case "$$scheme" in \
                        git) $($(DL_MODE)_GIT) && exit ;; \
                        svn) $($(DL_MODE)_SVN) && exit ;; \
                        cvs) $($(DL_MODE)_CVS) && exit ;; \
                        bzr) $($(DL_MODE)_BZR) && exit ;; \
                        file) $($(DL_MODE)_LOCALFILES) && exit ;; \
                        scp) $($(DL_MODE)_SCP) && exit ;; \
                        hg) $($(DL_MODE)_HG) && exit ;; \
                        *) $(call $(DL_MODE)_WGET,$(1),$(2)) && exit ;; \
                esac ; \
        fi ; \
        if test -n "$(call qstrip,$(BR2_BACKUP_SITE))" ; then \
                $(call $(DL_MODE)_WGET,$(BR2_BACKUP_SITE)/$(2),$(2)) && exit ; \
        fi ; \
        exit 1
    endef

So, we would need to catch missing-hash failure in:
- primary site,
- upstream loction,
- backup site.

But catching missing-hash failure needs a bit of a convoluted script syntax, something like:

    if $($(DL_MODE)_GIT); then \
        : success; \
        exit; \
    else \
        if [ ${?} -eq 1 ]; then \
            : normal failure; \
        else \
            : missing-hash failure; \
            exit 1; \
        fi; \
    fi

And repeat for all other download methods...

That's a bit cumbersome to write. Unless we introduce yet another macro that expands this code.

Your opinion?

Regards,
Yann E. MORIN.

Signature · 6 lines
.-----------------.--------------------.------------------.--------------------.
|  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.  |
'------------------------------^-------^------------------^--------------------'
#15 lore raw

v4 cover support/download: be more aggressive on missing hashes (branch yem/dl-hash-2)

Quoted 23 lines · Yann E. MORIN

Thomas, All,

On 2015-04-01 22:08 +0200, Thomas Petazzoni spake thusly:

Quoted 2 lines · Yann E. MORIN
Quoted 1 line

This series makes hashes mandatory when a .hash file exists.

[--SNIP--]

Quoted 17 lines

I applied this and was going to push it, but I believe there's still a problem.

If I change strace.hash so that there is no hash matching the tarball name of strace, then I get two times the error:

$ make strace-extract
ERROR: No hash found for strace-4.10.tar.xz
ERROR: No hash found for strace-4.10.tar.xz
package/pkg-generic.mk:73: recipe for target '/home/thomas/projets/buildroot/output/build/strace-4.10/.stamp_downloaded' failed

I haven't looked too deeply, but I believe it's because check-hash returns 3, so dl-wrapper exits with error code 1, which means that the pkg-download.mk logic concludes that the download from the upstream location has failed, so it retries with sources.buildroot.net, and the same thing happens.

Quoted 7 lines
Quoted 1 line

Is this expected?

That's at least the result I expect, yes.

I know this might look weird, indeed. However, I did not find a simple
way to avoid this.

I think this weird behaviour is acceptable (for now). It is indeed hard to
avoid, and anyway this is not something that a normal user would ever encounter.

In a perfect world the entire download logic would move to a script (which
admittedly would get a sh*tload of arguments). Then it would be much simpler to exit properly.

Regards,
Arnout

[snip]

Signature · 6 lines
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F
#16 lore raw

v4 cover support/download: be more aggressive on missing hashes (branch yem/dl-hash-2)

Quoted 40 lines · Arnout Vandecappelle
Quoted 23 lines · Yann E. MORIN

Thomas, All,

On 2015-04-01 22:08 +0200, Thomas Petazzoni spake thusly:

Quoted 2 lines · Yann E. MORIN
Quoted 1 line

This series makes hashes mandatory when a .hash file exists.

[--SNIP--]

Quoted 17 lines

I applied this and was going to push it, but I believe there's still a problem.

If I change strace.hash so that there is no hash matching the tarball name of strace, then I get two times the error:

$ make strace-extract
ERROR: No hash found for strace-4.10.tar.xz
ERROR: No hash found for strace-4.10.tar.xz
package/pkg-generic.mk:73: recipe for target '/home/thomas/projets/buildroot/output/build/strace-4.10/.stamp_downloaded' failed

I haven't looked too deeply, but I believe it's because check-hash returns 3, so dl-wrapper exits with error code 1, which means that the pkg-download.mk logic concludes that the download from the upstream location has failed, so it retries with sources.buildroot.net, and the same thing happens.

Quoted 7 lines
Quoted 1 line

Is this expected?

That's at least the result I expect, yes.

I know this might look weird, indeed. However, I did not find a simple
way to avoid this.

I think this weird behaviour is acceptable (for now). It is indeed hard to
avoid, and anyway this is not something that a normal user would ever encounter.

In a perfect world the entire download logic would move to a script (which
admittedly would get a sh*tload of arguments). Then it would be much simpler to exit properly.

Quoted 5 lines

Regards,
Arnout

[snip]

BTW, if you don't agree with this, you can still push patches 1 to 5 already.
They've got 2 reviews and they will be necessary anyway.

Regards,
Arnout

Signature · 6 lines
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F
#17 lore raw

v4 cover support/download: be more aggressive on missing hashes (branch yem/dl-hash-2)

In reply to Yann E. MORIN

Dear Yann E. MORIN,

Quoted 8 lines · Yann E. MORIN

Yann E. MORIN (7):
support/download: make hash file optional
package infra: do not check hashes when downloading from a repository support/download: add explicit no-hash support support/download: return different exit codes for different failures support/download: properly catch missing hashes support/download: always fail when there's no hash support/download: warn when there's no .hash file

Following the discussion we had, and Arnout comments, I applied this series as is. We can always think later of a way of solving the double error message problem, but it is not very important.

Thanks!

Thomas

Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
#18 lore raw

v4 cover support/download: be more aggressive on missing hashes (branch yem/dl-hash-2)

In reply to Yann E. MORIN

All,

On 2015-04-01 00:15 +0200, Yann E. MORIN spake thusly:

Quoted 1 line

This series makes hashes mandatory when a .hash file exists.

Please refrain from applying this series.

It does not play well with our autobuilders: we have a .hash file for external toolchains: that works well for pre-built toolchains from the well-known vendors, but that breaks for custom toolchains to be downloaded, since there's no hash for those.

I'll see what I can come up with.

Regards,
Yann E. MORIN.

Signature · 6 lines
.-----------------.--------------------.------------------.--------------------.
|  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.  |
'------------------------------^-------^------------------^--------------------'