Scripts and tools: gitian-build.py improvements and corrections #13998

pull hebasto wants to merge 3 commits into bitcoin:master from hebasto:gitian-build-fix changing 1 files +41 −21

hebasto commented at 10:04 PM on August 17, 2018: member
1. The Docker does not depend on apt-cacher-ng package. Ref: #14002.
2. Do not try to install the Docker if docker.service is detected on the system (e.g., the Docker was installed manually). Fix #13623 (comment) by Sjors.
3. Prevent the setting of more than one environment variable for the gitian-builder (an alternative to #13999). E.g., USE_LXC being set shadows USE_DOCKER; for details see gitian-builder/libexec/make-clean-vm:
```
VMSW=KVM
if [ -n "$USE_LXC" ]; then
    VMSW=LXC
elif [ -n "$USE_VBOX" ]; then
    VMSW=VBOX
elif [ -n "$USE_DOCKER" ]; then
    VMSW=DOCKER
fi
```
1. The gitian-builder/bin/gverify script returns the exit code 1 if a signature verification ends with 'BAD SIGNATURE' or 'MISMATCH' by design. This PR allows to see the verification results for all signatures without a premature fail of the gitian-build.py script. Ref: #14014.
fanquake added the label Scripts and tools on Aug 18, 2018
hebasto renamed this:
~~Scripts and tools: fix error message if Docker installed already~~
Scripts and tools: gitian-build.py improvements and corrections
on Aug 19, 2018
fanquake deleted a comment on Aug 19, 2018
DrahtBot commented at 8:37 AM on August 19, 2018: member


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #15236 (scripts and tools: Make --setup command independent by hebasto)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
hebasto force-pushed on Aug 19, 2018
hebasto force-pushed on Aug 19, 2018
hebasto force-pushed on Aug 19, 2018
hebasto force-pushed on Aug 19, 2018
laanwj requested review from ken2812221 on Sep 1, 2018
laanwj requested review from MarcoFalke on Sep 1, 2018

in contrib/gitian-build.py:170 in c45cf0ba56 outdated

 174 | -    if args.docker:
 175 | -        os.environ['USE_DOCKER'] = '1'
 176 | -    elif not args.kvm:
 177 | +    args.lxc = 'lxc' in args.virtualization
 178 | +    args.kvm = 'kvm' in args.virtualization
 179 | +    args.docker = 'docker' in args.virtualization

ken2812221 commented at 4:47 PM on September 1, 2018:

I think == would be better. To avoid lxckvm

in contrib/gitian-build.py:15 in c45cf0ba56 outdated

  13 | +        programs += ['apt-cacher-ng', 'lxc', 'debootstrap']
  14 |      if args.kvm:
  15 | -        programs += ['python-vm-builder', 'qemu-kvm', 'qemu-utils']
  16 | -    elif args.docker:
  17 | +        programs += ['apt-cacher-ng', 'python-vm-builder', 'qemu-kvm', 'qemu-utils']
  18 | +    if args.docker and not os.path.isfile('/lib/systemd/system/docker.service'):

ken2812221 commented at 4:50 PM on September 1, 2018:

I think elif is better here.

in contrib/gitian-build.py:35 in c45cf0ba56 outdated

  31 | @@ -32,10 +32,10 @@ def setup():
  32 |          subprocess.check_call(['git', 'clone', 'https://github.com/bitcoin/bitcoin.git'])
  33 |      os.chdir('gitian-builder')
  34 |      make_image_prog = ['bin/make-base-vm', '--suite', 'bionic', '--arch', 'amd64']
  35 | +    if args.lxc:
  36 | +        make_image_prog += ['--lxc']
  37 |      if args.docker:

ken2812221 commented at 10:55 AM on September 5, 2018:

elif

ken2812221 approved
ken2812221 commented at 11:01 AM on September 5, 2018: contributor

utACK c45cf0ba56d1405d8c6a87d899a8390c5a6e4b0b. Some nits and please squash.

in contrib/gitian-build.py:147 in c45cf0ba56 outdated

 142 | @@ -143,9 +143,8 @@ def main():
 143 |      parser.add_argument('-o', '--os', dest='os', default='lwm', help='Specify which Operating Systems the build is for. Default is %(default)s. l for Linux, w for Windows, m for MacOS')
 144 |      parser.add_argument('-j', '--jobs', dest='jobs', default='2', help='Number of processes to use. Default %(default)s')
 145 |      parser.add_argument('-m', '--memory', dest='memory', default='2000', help='Memory to allocate in MiB. Default %(default)s')
 146 | -    parser.add_argument('-k', '--kvm', action='store_true', dest='kvm', help='Use KVM instead of LXC')
 147 | -    parser.add_argument('-d', '--docker', action='store_true', dest='docker', help='Use Docker instead of LXC')
 148 | -    parser.add_argument('-S', '--setup', action='store_true', dest='setup', help='Set up the Gitian building environment. Uses LXC. If you want to use KVM, use the --kvm option. Only works on Debian-based systems (Ubuntu, Debian)')
 149 | +    parser.add_argument('-V', '--virtualization', dest='virtualization', default='lxc', help='Specify virtualization technology to use: lxc for LXC, kvm for KVM, docker for Docker. Default is %(default)s')

MarcoFalke commented at 12:28 PM on September 5, 2018:

Just noting that external documentation would have to be updated to reflect this change.

hebasto commented at 6:37 PM on September 5, 2018:

All related documentation which describes this script options is located in the https://github.com/bitcoin-core/docs repository. Therefore, IMO, there is nothing to add to this PR regarding docs. Or did I miss something?

I'll submit an appropriate PR to the https://github.com/bitcoin-core/docs repository in a timely manner.

MarcoFalke commented at 8:53 PM on May 17, 2019:

This can be achieved without a breaking change via

args.lxc = not args.kvm and not args.docker

hebasto force-pushed on Sep 5, 2018
hebasto commented at 5:23 PM on September 5, 2018: member

@ken2812221 Thank you for your review.

Some nits and please squash.

All nits are fixed and commits are squashed.
hebasto force-pushed on Sep 5, 2018
DrahtBot added the label Needs rebase on Sep 5, 2018
hebasto force-pushed on Sep 6, 2018
hebasto commented at 6:02 AM on September 6, 2018: member

Rebased.
DrahtBot removed the label Needs rebase on Sep 6, 2018
ken2812221 commented at 12:10 AM on September 17, 2018: contributor

re-utACK be88ede
hebasto commented at 6:11 PM on October 20, 2018: member

@fanquake #14014 has been combined into this PR. Would you mind reviewing it?
hebasto commented at 7:03 PM on November 2, 2018: member

@fanquake May I ask you to restart travis against this PR?
ken2812221 commented at 8:09 PM on November 2, 2018: contributor

utACK dbd217be69ab6453f0adeb52cbacdace78406a45 travis restarted
in contrib/gitian-build.py:129 in dbd217be69 outdated
```
 129 |      print('\nVerifying v'+args.version+' Signed Windows\n')
 130 | -    subprocess.check_call(['bin/gverify', '-v', '-d', '../gitian.sigs/', '-r', args.version+'-win-signed', '../bitcoin/contrib/gitian-descriptors/gitian-win-signer.yml'])
 131 | +    subprocess.call(['bin/gverify', '-v', '-d', '../gitian.sigs/', '-r', args.version+'-win-signed', '../bitcoin/contrib/gitian-descriptors/gitian-win-signer.yml'])
 132 |      print('\nVerifying v'+args.version+' Signed MacOS\n')
 133 | -    subprocess.check_call(['bin/gverify', '-v', '-d', '../gitian.sigs/', '-r', args.version+'-osx-signed', '../bitcoin/contrib/gitian-descriptors/gitian-osx-signer.yml'])
 134 | +    subprocess.call(['bin/gverify', '-v', '-d', '../gitian.sigs/', '-r', args.version+'-osx-signed', '../bitcoin/contrib/gitian-descriptors/gitian-osx-signer.yml'])
```
MarcoFalke commented at 9:56 PM on November 2, 2018:

Why should this not fail if there is a mismatch?
hebasto commented at 10:09 PM on November 2, 2018:

The gitian-builder/bin/gverify script returns the exit code 1 if a signature verification ends with 'BAD SIGNATURE' or 'MISMATCH' by design.

From the Python docs:

subprocess.check_call():

If the return code was zero then return, otherwise raise CalledProcessError.

subprocess.call():

Wait for command to complete, then return the returncode attribute.
MarcoFalke commented at 10:17 PM on November 2, 2018:

I understand that by this change the return code is ignored, but I'd like to understand why.

If the signatures are bad or the hashes mismatch, the verification should fail.

hebasto commented at 10:24 PM on November 2, 2018:

I mean script fails. You cannot see the result of verifying of the signatures that remain. With this commit script will not fail and you will see the results for all signatures.

MarcoFalke commented at 5:22 PM on November 15, 2018:

Fine, if you want to print the result for all signatures, but please don't change the behaviour (fail on non-zero exit code) of the script.
hebasto force-pushed on Nov 22, 2018
hebasto commented at 11:43 PM on November 22, 2018: member

@MarcoFalke Thank you for your review.

Fine, if you want to print the result for all signatures, but please don't change the behaviour (fail on non-zero exit code) of the script.

Fixed. Would you mind re-reviewing?

Fix Docker related issues for gitian-build.py

The Docker does not depend on apt-cacher-ng package.
Do not try to install the Docker if docker.service is detected on the
system (e.g., the Docker was installed manually).
Also small style corrections were applied.

cbbd98863b

Set/unset USE_LXC, USE_VBOX, USE_DOCKER explicitly

This prevents the setting of more than one environment variable for the
gitian-builder (e.g., USE_LXC being set shadows USE_DOCKER; for details
see gitian-builder/libexec/make-clean-vm).

4c56a798c0

Fix gitian-build.py --verify option

The gitian-builder/bin/gverify script returns the exit code 1 if a
signature verification ends with 'BAD SIGNATURE' or 'MISMATCH' by
design. This commit allows to see the verification results for all
signatures without a premature fail of the gitian-build.py script.

0f22a0cf2f

hebasto force-pushed on May 18, 2019
hebasto commented at 4:52 PM on May 18, 2019: member

Rebased. @MarcoFalke

This can be achieved without a breaking change via args.lxc = not args.kvm and not args.docker

Dropped new --virtualization option. So, no breaking changes now.

Also "do not warn about macOS build on setup" feature dropped in favor of #15236.

Commit messages have been brushed. PR description updated.
hebasto commented at 5:15 PM on May 18, 2019: member

@Sjors Would you mind reviewing this PR regarding your comment?
MarcoFalke merged this on May 20, 2019
MarcoFalke closed this on May 20, 2019
MarcoFalke referenced this in commit f49b8d4783 on May 20, 2019
hebasto deleted the branch on May 20, 2019
sidhujag referenced this in commit 19c473999e on May 20, 2019
MarcoFalke referenced this in commit 2d1583ee6a on May 20, 2019
UdjinM6 referenced this in commit 84b3c0b1ee on Sep 1, 2021
UdjinM6 referenced this in commit 5baf92492b on Sep 2, 2021
UdjinM6 referenced this in commit 10c56c07b9 on Sep 2, 2021
UdjinM6 referenced this in commit 0f8ae5e6e6 on Sep 2, 2021
UdjinM6 referenced this in commit 678e2af0f3 on Sep 2, 2021
UdjinM6 referenced this in commit f512e704d5 on Sep 2, 2021
MarcoFalke locked this on Dec 16, 2021

Contributors

Labels

Linked (view graph)

#13623 Migrate gitian-build.sh to python #13999 Scripts and tools: Dedicated --virtualization option #14002 Scripts and tools: fixed Docker dependencies #14014 Scripts and tools: fix gitian-build.py --verify option #15236 scripts and tools: Make --setup command independent

Scripts and tools: gitian-build.py improvements and corrections #13998

Conflicts