script: fixed wget call in gitian-build.py #17671

willyko commented at 12:31 AM on December 5, 2019: contributor

Missing comma makes the gitian-builder script to download osslsigncode-2.0.tar.gz as osslsigncode-2.0.tar.gz-N, which makes the subsequent calls fail when building window binaries

willyko commented at 1:16 AM on December 5, 2019: contributor

When I tested that wget line, the -P -O and -N options don't seem to work together. -O seems to override -P and using -O and -N together gives the following warning:

WARNING: timestamping does nothing in combination with -O. See the manual
for details.

MarcoFalke commented at 1:43 AM on December 5, 2019: member

Whatever you use, please use the same command as in the doc/release-process.md and keep your commits squashed according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

DrahtBot added the label Build system on Dec 5, 2019

DrahtBot added the label Scripts and tools on Dec 5, 2019

fanquake removed the label Build system on Dec 5, 2019

willyko commented at 3:45 AM on December 5, 2019: contributor

In https://github.com/bitcoin/bitcoin/blob/master/doc/release-process.md#L119 The wget command does not place the tar.gz in the input folder. Should I open an issue or squash commit to include the change to the release-process.md too? my wget version: GNU Wget 1.17.1 built on linux-gnu

sidhujag commented at 4:17 AM on December 5, 2019: none

is there a reason to use -P prefix and not just write to the right folder/path

in contrib/gitian-build.py:54 in 78b17a51fd outdated

  50 | @@ -51,7 +51,7 @@ def build():
  51 |      os.chdir('gitian-builder')
  52 |      os.makedirs('inputs', exist_ok=True)
  53 |  
  54 | -    subprocess.check_call(['wget', '-O' 'osslsigncode-2.0.tar.gz' '-N', '-P', 'inputs', 'https://github.com/mtrojnar/osslsigncode/archive/2.0.tar.gz'])
  55 | +    subprocess.check_call(['wget', '-O', 'inputs/osslsigncode-2.0.tar.gz', 'https://github.com/mtrojnar/osslsigncode/archive/2.0.tar.gz'])

laanwj commented at 9:27 AM on December 5, 2019:

This is not just a typo but an actual bug-fix, right? (at the least it really changes the meaning of the code)

willyko commented at 9:32 AM on December 5, 2019:

right. a bug caused by typo?

laanwj commented at 10:01 AM on December 5, 2019:

possibly, it's not really useful to guess usually descriptions such as "typo" are used for harmless mistypings in comments in any case if you fix actual problems it's better to say so in the PR title so that it's taken more seriously (and so that people can review the behavior change)

laanwj commented at 10:02 AM on December 5, 2019: member

ACK after squash and improving PR title

Fixed wget call in gitian-build.py b11d35b5e2

willyko force-pushed on Dec 5, 2019

willyko renamed this:
~~Fixed typo in gitian-build.py~~
script: fixed wget call in gitian-build.py
on Dec 5, 2019

laanwj commented at 10:26 AM on December 5, 2019: member

much better ! ACK b11d35b5e2dd09ab816d688d8ac0264b43f7f844

in doc/release-process.md:119 in b11d35b5e2

 115 | @@ -116,7 +116,7 @@ Ensure gitian-builder is up-to-date:
 116 |  
 117 |      pushd ./gitian-builder
 118 |      mkdir -p inputs
 119 | -    wget -O osslsigncode-2.0.tar.gz -P inputs https://github.com/mtrojnar/osslsigncode/archive/2.0.tar.gz
 120 | +    wget -O inputs/osslsigncode-2.0.tar.gz https://github.com/mtrojnar/osslsigncode/archive/2.0.tar.gz

promag commented at 2:50 PM on December 5, 2019:

Just saying that with -P inputs -O osslsigncode-2.0.tar.gz the above mkdir could be removed - at least on my system. I prefer this way.

willyko commented at 6:35 PM on December 5, 2019:

May I get your wget version? I'm running on bionic GNU wget 1.17.1 and for my wget, the -P gets ignored when there's -O When I run wget -P inputs -O osslsigncode-2.0.tar.gz https://github.com/mtrojnar/osslsigncode/archive/2.0.tar.gz, it does not place the output file osslsigncode-2.0.tar.gz in the inputs directory regardless of whether inputs directory's existence

laanwj commented at 8:41 AM on December 6, 2019:

I very much prefer making the directory explicitly as it is done now instead of relying on implicit wget behavior.

promag commented at 2:51 PM on December 5, 2019: member

ACK b11d35b5e2dd09ab816d688d8ac0264b43f7f844.

hebasto approved

hebasto commented at 11:01 AM on December 7, 2019: member

ACK b11d35b5e2dd09ab816d688d8ac0264b43f7f844, I have reviewed the code and it looks OK, I agree it can be merged.

Ref: GNU Wget Manual; see the following options:

‘-O file’ / ‘--output-document=file’
‘-N’ / ‘--timestamping’
‘-P prefix’ / ‘--directory-prefix=prefix’

Also checked usage of wget in other places of the code base.

MarcoFalke referenced this in commit 5622d8f315 on Dec 7, 2019

MarcoFalke merged this on Dec 7, 2019

MarcoFalke closed this on Dec 7, 2019

sidhujag referenced this in commit f259ea4c27 on Dec 7, 2019

UdjinM6 referenced this in commit 7f8b8b31e4 on Jan 1, 2020

UdjinM6 referenced this in commit cbf9c54a1d on Jan 10, 2020

barrystyle referenced this in commit b302a7d97a on Jan 22, 2020

MarkLTZ referenced this in commit 71bc1598ab on Apr 10, 2020

sidhujag referenced this in commit a0b9726205 on Nov 10, 2020

random-zebra referenced this in commit 41064cf016 on Mar 4, 2021

ckti referenced this in commit 8acba9e7d9 on Mar 28, 2021

ckti referenced this in commit 531822330a on Mar 28, 2021

MarcoFalke locked this on Dec 16, 2021