[Buildroot] wget download: 'scheme missing' results in empty output file

[Buildroot] wget download: 'scheme missing' results in empty output file

[Thomas De Schampheleire]

Hi Yann, all,

When a package does not (by error) define FOO_SITE, and the source
archive is not already present, the wget download helper will be
invoked with as URL /foo-xxx.tar.gz. wget reports 'Scheme missing' and
bails out, but _returns with a successful error code (0)_.

As a result, the download helper happily continues with hash checking
and finally with the atomic mv of the temporary download file (which
has been created but is still empty) to the final output location.

Because as far as the download helper is concerned, everything went
successfully, also the build continues (but obviously fails further
on).

While I think wget should not return 0 on such a problem, I think we
should try and prevent such problems. One way is to expect the
downloaded file size to be greater than 0. If that requirement is not
met, the download helper should bail out.

A patch like the below fixes the problem:

diff --git a/support/download/dl-wrapper b/support/download/dl-wrapper
--- a/support/download/dl-wrapper
+++ b/support/download/dl-wrapper
@@ -87,6 +87,12 @@ main() {
         exit 1
     fi

+    if ! [ -s "${output}" ]; then
+        error "empty download detected\n"
+        rm -rf "${tmpd}"
+        exit 1
+    fi
+
     # cd back to free the temp-dir, so we can remove it later
     cd "${OLDPWD}"



This clearly assumes that no package tries to download a file with size 0.

If you agree with such a change I can send a proper patch.

Thanks,
Thomas

[Buildroot] wget download: 'scheme missing' results in empty output file

[Peter Korsgaard]

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

 > Hi Yann, all,
 > When a package does not (by error) define FOO_SITE, and the source
 > archive is not already present, the wget download helper will be
 > invoked with as URL /foo-xxx.tar.gz. wget reports 'Scheme missing' and
 > bails out, but _returns with a successful error code (0)_.

That sounds like a wget bug. What wget version are you using?

It works here:

wget /foo-xxx.tar.gz ; echo $?
/foo-xxx.tar.gz: Scheme missing.
1

wget --version | head
GNU Wget 1.16.3 built on linux-gnu.

+digest +https +ipv6 +iri +large-file +nls +ntlm +opie +psl +ssl/gnutls 

Wgetrc: 
    /etc/wgetrc (system)
Locale: 
    /usr/share/locale 
Compile: 
    gcc -DHAVE_CONFIG_H -DSYSTEM_WGETRC="/etc/wgetrc" 

-- 
Bye, Peter Korsgaard

[Buildroot] wget download: 'scheme missing' results in empty output file

[Thomas De Schampheleire]

On Thu, Feb 25, 2016 at 4:40 PM, Peter Korsgaard <peter@korsgaard.com> wrote:
>>>>>> "Thomas" == Thomas De Schampheleire <patrickdepinguin@gmail.com> writes:
>
>  > Hi Yann, all,
>  > When a package does not (by error) define FOO_SITE, and the source
>  > archive is not already present, the wget download helper will be
>  > invoked with as URL /foo-xxx.tar.gz. wget reports 'Scheme missing' and
>  > bails out, but _returns with a successful error code (0)_.
>
> That sounds like a wget bug. What wget version are you using?
>
> It works here:
>
> wget /foo-xxx.tar.gz ; echo $?
> /foo-xxx.tar.gz: Scheme missing.
> 1
>
> wget --version | head
> GNU Wget 1.16.3 built on linux-gnu.
>
> +digest +https +ipv6 +iri +large-file +nls +ntlm +opie +psl +ssl/gnutls
>
> Wgetrc:
>     /etc/wgetrc (system)
> Locale:
>     /usr/share/locale
> Compile:
>     gcc -DHAVE_CONFIG_H -DSYSTEM_WGETRC="/etc/wgetrc"
>

This is with wget 1.12 from 2009 :)

Ok, so probably it's fixed meanwhile, but regardless what about the
patch which could cover other issues in such tools (not only wget) ?

[Buildroot] wget download: 'scheme missing' results in empty output file

[Peter Korsgaard]

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

Hi,

 > This is with wget 1.12 from 2009 :)

Yeah, I guessed this was some kind of crufty RHEL thing ;)

 > Ok, so probably it's fixed meanwhile, but regardless what about the
 > patch which could cover other issues in such tools (not only wget) ?

Small workarounds for legacy tools are ok, as long as they don't add too
much complexity and the issue / version they work around is documented
so we can later remove them again if needed.

-- 
Venlig hilsen,
Peter Korsgaard 

[Buildroot] wget download: 'scheme missing' results in empty output file

[Thomas Petazzoni]

Thomas,

On Thu, 25 Feb 2016 14:19:56 +0100, Thomas De Schampheleire wrote:

> When a package does not (by error) define FOO_SITE, and the source
> archive is not already present, the wget download helper will be
> invoked with as URL /foo-xxx.tar.gz. wget reports 'Scheme missing' and
> bails out, but _returns with a successful error code (0)_.

Shouldn't we simply be checking in the package infrastructure that
FOO_SITE is not empty?

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

[Buildroot] wget download: 'scheme missing' results in empty output file

[Thomas De Schampheleire]

On Thu, Feb 25, 2016 at 5:07 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Thomas,
>
> On Thu, 25 Feb 2016 14:19:56 +0100, Thomas De Schampheleire wrote:
>
>> When a package does not (by error) define FOO_SITE, and the source
>> archive is not already present, the wget download helper will be
>> invoked with as URL /foo-xxx.tar.gz. wget reports 'Scheme missing' and
>> bails out, but _returns with a successful error code (0)_.
>
> Shouldn't we simply be checking in the package infrastructure that
> FOO_SITE is not empty?
>

When a PRIMARY_MIRROR is set and there is a 'guarantee' or expectation
that each file is present on that mirror, FOO_SITE is not really used
and could be empty for proprietary packages, since there is no actual
proper value. Sure one could fill in a bogus value, but it's not very
nice.

Also, for packages where the source is included with buildroot, like
makedevs, FOO_SITE is empty too.

/Thomas

[Buildroot] wget download: 'scheme missing' results in empty output file

[Peter Korsgaard]

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

Hi,

 >> Shouldn't we simply be checking in the package infrastructure that
 >> FOO_SITE is not empty?

> When a PRIMARY_MIRROR is set and there is a 'guarantee' or expectation
 > that each file is present on that mirror, FOO_SITE is not really used
 > and could be empty for proprietary packages, since there is no actual
 > proper value. Sure one could fill in a bogus value, but it's not very
 > nice.

You could set _SITE to your PRIMARY_MIRROR, which is really where those
tarballs (should be) are located.

 > Also, for packages where the source is included with buildroot, like
 > makedevs, FOO_SITE is empty too.

.. but those also don't define _SOURCE / _VERSION, so wget isn't called
;)

-- 
Bye, Peter Korsgaard

[Buildroot] wget download: 'scheme missing' results in empty output file

[Thomas Petazzoni]

Dear Peter Korsgaard,

On Thu, 25 Feb 2016 17:34:14 +0100, Peter Korsgaard wrote:

>  > Also, for packages where the source is included with buildroot, like
>  > makedevs, FOO_SITE is empty too.
> 
> .. but those also don't define _SOURCE / _VERSION, so wget isn't called
> ;)

Yeah, but I was proposing to check for an empty FOO_SITE in the package
infrastructure itself, so we're not at the point where wget gets called.

But to answer Thomas: for cases like makedevs, we can special case such
packages (their _SOURCE is empty, or something like this).

Best regards,

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

[Buildroot] wget download: 'scheme missing' results in empty output file

[Yann E. MORIN]

Thomas?, All,

On 2016-02-25 18:01 +0100, Thomas Petazzoni spake thusly:
> Dear Peter Korsgaard,
> 
> On Thu, 25 Feb 2016 17:34:14 +0100, Peter Korsgaard wrote:
> 
> >  > Also, for packages where the source is included with buildroot, like
> >  > makedevs, FOO_SITE is empty too.
> > 
> > .. but those also don't define _SOURCE / _VERSION, so wget isn't called
> > ;)
> 
> Yeah, but I was proposing to check for an empty FOO_SITE in the package
> infrastructure itself, so we're not at the point where wget gets called.

What about:

    diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
    index e22babb..64c6fdc 100644
    --- a/package/pkg-generic.mk
    +++ b/package/pkg-generic.mk
    @@ -436,7 +436,7 @@ endif
     $(2)_ALL_DOWNLOADS = \
        $$(foreach p,$$($(2)_SOURCE) $$($(2)_PATCH) $$($(2)_EXTRA_DOWNLOADS),\
            $$(if $$(findstring ://,$$(p)),$$(p),\
    -$$($(2)_SIT            E)/$$(p)))
    +$$(if $$($(2)_SITE),$$($(2)_SITE)/$$(p))))
 
     ifndef $(2         )_SITE
      ifdef $(3)_SITE

> But to answer Thomas: for cases like makedevs, we can special case such
> packages (their _SOURCE is empty, or something like this).

Indeed, they are already covered by the existing exclusions.

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] wget download: 'scheme missing' results in empty output file

[Thomas De Schampheleire]

On Thu, Feb 25, 2016 at 9:32 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
> Thomas?, All,
>
> On 2016-02-25 18:01 +0100, Thomas Petazzoni spake thusly:
>> Dear Peter Korsgaard,
>>
>> On Thu, 25 Feb 2016 17:34:14 +0100, Peter Korsgaard wrote:
>>
>> >  > Also, for packages where the source is included with buildroot, like
>> >  > makedevs, FOO_SITE is empty too.
>> >
>> > .. but those also don't define _SOURCE / _VERSION, so wget isn't called
>> > ;)
>>
>> Yeah, but I was proposing to check for an empty FOO_SITE in the package
>> infrastructure itself, so we're not at the point where wget gets called.
>
> What about:
>
>     diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
>     index e22babb..64c6fdc 100644
>     --- a/package/pkg-generic.mk
>     +++ b/package/pkg-generic.mk
>     @@ -436,7 +436,7 @@ endif
>      $(2)_ALL_DOWNLOADS = \
>         $$(foreach p,$$($(2)_SOURCE) $$($(2)_PATCH) $$($(2)_EXTRA_DOWNLOADS),\
>             $$(if $$(findstring ://,$$(p)),$$(p),\
>     -$$($(2)_SIT            E)/$$(p)))
>     +$$(if $$($(2)_SITE),$$($(2)_SITE)/$$(p))))
>
>      ifndef $(2         )_SITE
>       ifdef $(3)_SITE

I tried this, but it does not yield the desired end-result: no file
will be downloaded, the download step 'succeeds', and the extract step
fails because it can't find the expected file.

I found another clean and simple solution to my problem:

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -442,6 +442,8 @@ endif
 ifndef $(2)_SITE
  ifdef $(3)_SITE
   $(2)_SITE = $$($(3)_SITE)
+ else
+  $(2)_SITE = undefined
  endif
 endif

A similar 'undefined' trick is done to $(2)_VERSION.
Basically, either site is set, or it defaults to 'undefined'.
When a download is not attempted (e.g. source in buildroot, ...) there
is no change, if a download is attempted then it will fail due to
'undefined'.

What do you think?

Thanks,
Thomas

[Buildroot] wget download: 'scheme missing' results in empty output file

[Thomas Petazzoni]

Hello,

On Thu, 25 Feb 2016 14:19:56 +0100, Thomas De Schampheleire wrote:
> Hi Yann, all,
> 
> When a package does not (by error) define FOO_SITE, and the source
> archive is not already present, the wget download helper will be
> invoked with as URL /foo-xxx.tar.gz. wget reports 'Scheme missing' and
> bails out, but _returns with a successful error code (0)_.
> 
> As a result, the download helper happily continues with hash checking
> and finally with the atomic mv of the temporary download file (which
> has been created but is still empty) to the final output location.
> 
> Because as far as the download helper is concerned, everything went
> successfully, also the build continues (but obviously fails further
> on).
> 
> While I think wget should not return 0 on such a problem, I think we
> should try and prevent such problems. One way is to expect the
> downloaded file size to be greater than 0. If that requirement is not
> met, the download helper should bail out.
> 
> A patch like the below fixes the problem:

Instead of this, I just submitted a patch that verifies that SITE is
not empty if SOURCE is not empty. See
https://patchwork.ozlabs.org/patch/643725/. Consequently, I've marked
your patch as Superseded in patchwork.

Thanks,

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