Migrate gitian-build.sh to python

ken2812221 commented at 2:32 pm on July 10, 2018: contributor

Fixes #13620

Rename Mac OSX to MacOS, rename option from ‘x’ to ’m’
Fix a bug from https://github.com/bitcoin/bitcoin/blob/b641f60425674d737d77abd8c49929d953ea4154/contrib/gitian-build.sh#L338-L342

ken2812221 force-pushed on Jul 10, 2018

laanwj added the label Build system on Jul 10, 2018

laanwj commented at 3:17 pm on July 10, 2018: member

Thanks // Concept ACK, code looks good to me at first glance but ofcourse needs manual testing (as it’s not tested by the test framework).

MarcoFalke commented at 3:45 pm on July 10, 2018: member

Could remove the bash script in the same commit?

ken2812221 force-pushed on Jul 10, 2018

ken2812221 renamed this:
~~[WIP] Migrate gitian-build.sh to python~~
Migrate gitian-build.sh to python
on Jul 10, 2018

ken2812221 force-pushed on Jul 10, 2018

DrahtBot commented at 4:27 pm on July 10, 2018: member

#13643 (Switch to NSIS 3.03 to avoid DLL hijacking by h4x3rotab)
#13368 (Update gitian-build.sh for docker by achow101)
#13171 (Change gitian-descriptors to use bionic instead by ken2812221)

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.

jtimon commented at 8:18 pm on July 10, 2018: contributor

Concept ACK

dongcarl commented at 9:07 pm on July 10, 2018: member

As gitian-build.sh is mostly calling other executables, I think bash/sh is still the most sensible/readable/least-dependencies choice. According to #13620 there are edge cases to fix, sure, but I’m not convinced that there’s a need for a python rewrite.

Static analysis tools run against shell scripts in the check pipeline can eliminate/mitigate edge cases. I’d recommend: https://github.com/koalaman/shellcheck

ken2812221 force-pushed on Jul 11, 2018

laanwj commented at 11:34 am on July 11, 2018: member

As gitian-build.sh is mostly calling other executables, I think bash/sh is still the most sensible/readable/least-dependencies choice.

The point is that most of the developers in this project have a much easier time reviewing and maintaining python code. Shell script has turned out to be a continuous struggle against shell differences, bad performance, bugs and other edge cases due to obscure quoting practices, and so on.

You might disagree personally–obviously everything is possible with shell script as well, but porting this to python is more of a social consensus. (as for dependencies, I don’t think that is a big thing here, Python 3 is already required to build the project and this requires no external modules)

MarcoFalke commented at 11:37 am on July 11, 2018: member

Also, the resulting python script is a lot shorter. Means less code weight to maintain in the future :)

in contrib/gitian-build.py:138 in bc284da7f0 outdated

125+    parser.add_argument('-m', '--memory', dest='memory', default=2000, help='Memory to allocate in MiB. Default %(default)s')
126+    parser.add_argument('-k', '--kvm', action='store_true', dest='kvm', help='Use KVM instead of LXC')
127+    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)')
128+    parser.add_argument('-d', '--detach-sign', action='store_true', dest='detach_sign', help='Create the assert file for detached signing. Will not commit anything.')
129+    parser.add_argument('-n', '--no-commit', action='store_false', dest='commit_files', help='Do not commit anything to git')
130+    parser.add_argument('signer', help='GPG signer to sign each build assert file')

mcdallas commented at 4:34 pm on July 11, 2018:

You can add the parameter nargs=1 to make signer and version mandatory and skip lines 155-162. It will produce a list of one item. See https://docs.python.org/3/library/argparse.html#nargs

achow101 commented at 9:24 pm on July 11, 2018:

This isn’t necessary since signer and version are positional arguments so if they aren’t provided then an error will always be thrown. The checks later are still necessary since you can still pass in the empty string as an argument regardless of nargs.

mcdallas commented at 10:00 pm on July 11, 2018:

Oh I missed the point of the checks. Still if we want to guard against empty string we can use the type argument with a custom callable (saves a few lines)

ken2812221 force-pushed on Jul 12, 2018

ken2812221 commented at 2:45 am on July 12, 2018: contributor

Update: exit 1 if signer or version is empty.

in contrib/gitian-build.py:179 in a67812d15e outdated

161+    if args.version == '':
162+        print(script_name+': Missing version.')
163+        print('Try '+script_name+' --help for more information')
164+        exit(1)
165+
166+    # Add leading 'v' for tags

jb55 commented at 6:05 am on July 13, 2018:

is this option necessary? if there’s a v you could just include it in the argument. I found this confusing when I ran the command, it was trying to checkout vmaster because I didn’t have the --commit option.

ken2812221 commented at 7:07 am on July 13, 2018:

The version without ‘v’ prefix is still used in gsign.

in contrib/gitian-build.py:186 in a67812d15e outdated

168+    print(args.commit)
169+
170+    if args.setup:
171+        setup()
172+
173+    os.chdir('bitcoin')

jb55 commented at 6:06 am on July 13, 2018:

this assumes you would be running the command from outside the bitcoin directory. is this correct? I found I had to cd .. and then run ./bitcoin/contrib/gitian-builder.py to make the command work?

ken2812221 commented at 6:28 am on July 13, 2018:

From the instruction from https://github.com/bitcoin-core/docs/blob/master/gitian-building.md , you should copy the script to the same level as bitcoin folder

jb55 commented at 4:59 pm on July 13, 2018:

ah ok thanks

in contrib/gitian-build.py:191 in a67812d15e outdated

173+    os.chdir('bitcoin')
174+    subprocess.check_call(['git', 'fetch'])
175+    subprocess.check_call(['git', 'checkout', args.commit])
176+    os.chdir(workdir)
177+
178+    if args.build:

jb55 commented at 6:11 am on July 13, 2018:

perhaps we could default to build? or at least throw an error when all three are not selected?

jb55 changes_requested

jb55 commented at 6:12 am on July 13, 2018: member

I only tested some of the basic ergonomics since I don’t have the gitian builder set up on my machine yet.

fanquake commented at 9:57 am on July 13, 2018: member

Using a67812d if I pass --setup and already have one of the required repos cloned I see:

 0Processing triggers for ureadahead (0.100.0-20) ...
 1Processing triggers for ufw (0.35-5) ...
 2fatal: destination path 'gitian.sigs' already exists and is not an empty directory.
 3Traceback (most recent call last):
 4  File "./gitian-build.py", line 188, in <module>
 5    main()
 6  File "./gitian-build.py", line 171, in main
 7    setup()
 8  File "./gitian-build.py", line 16, in setup
 9    subprocess.check_call(['git', 'clone', 'https://github.com/bitcoin-core/gitian.sigs.git'])
10  File "/usr/lib/python3.6/subprocess.py", line 291, in check_call
11    raise CalledProcessError(retcode, cmd)
12subprocess.CalledProcessError: Command '['git', 'clone', 'https://github.com/bitcoin-core/gitian.sigs.git']' returned non-zero exit status 128.
13ubuntu@ubuntu:~$ ls

Removed the repos and re-ran with --setup:

 0|               E |
 1+----[SHA256]-----+
 2sudo: debootstrap: command not found
 3Traceback (most recent call last):
 4  File "./gitian-build.py", line 188, in <module>
 5    main()
 6  File "./gitian-build.py", line 171, in main
 7    setup()
 8  File "./gitian-build.py", line 23, in setup
 9    subprocess.check_call(make_image_prog)
10  File "/usr/lib/python3.6/subprocess.py", line 291, in check_call
11    raise CalledProcessError(retcode, cmd)
12subprocess.CalledProcessError: Command '['bin/make-base-vm', '--suite', 'trusty', '--arch', 'amd64', '--lxc']' returned non-zero exit status 1.

--setup --kvm

 0invoke-rc.d: policy-rc.d denied execution of restart.
 1
 2Configuration file '/etc/sudoers'
 3 ==> Modified (by you or by a script) since installation.
 4 ==> Package distributor has shipped an updated version.
 5   What would you like to do about it ?  Your options are:
 6    Y or I  : install the package maintainer's version
 7    N or O  : keep your currently-installed version
 8      D     : show the differences between the versions
 9      Z     : start a shell to examine the situation
10 The default action is to keep your current version.
11*** sudoers (Y/I/N/O/D/Z) [default=N] ? dpkg: error processing package sudo (--configure):
12 EOF on stdin at conffile prompt
13invoke-rc.d: policy-rc.d denied execution of restart.
14Errors were encountered while processing:
15 sudo
16E: Sub-process /usr/bin/dpkg returned an error code (1)
17
18Traceback (most recent call last):
19  File "./gitian-build.py", line 188, in <module>
20    main()
21  File "./gitian-build.py", line 171, in main
22    setup()
23  File "./gitian-build.py", line 23, in setup
24    subprocess.check_call(make_image_prog)
25  File "/usr/lib/python3.6/subprocess.py", line 291, in check_call
26    raise CalledProcessError(retcode, cmd)
27subprocess.CalledProcessError: Command '['bin/make-base-vm', '--suite', 'trusty', '--arch', 'amd64']' returned non-zero exit status 1.

ken2812221 force-pushed on Jul 13, 2018

ken2812221 commented at 11:48 am on July 13, 2018: contributor

I don’t know how to deal with KVM errors since I haven’t used KVM ever, I just follow the same command with the original script. Can anyone give a hint?

fanquake commented at 11:44 pm on July 15, 2018: member

@ken2812221 Now that #13368 has been merged, can you update the new Python script to add Docker support. I will retest the changes here shortly.

ken2812221 force-pushed on Jul 16, 2018

DrahtBot added the label Needs rebase on Jul 16, 2018

DrahtBot commented at 11:16 am on July 16, 2018: member

Migrate gitian-build.sh to python 78f06e4af7

ken2812221 force-pushed on Jul 16, 2018

fanquake removed the label Needs rebase on Jul 17, 2018

Sjors commented at 4:17 pm on July 17, 2018: member

This is great, especially not having to worry about the exact sequence of arguments (the bash script had some quirks).

I’m trying to sign master on a Bionic VM (haven’t tried a Debian 9 host).

0# git clone bitcoin, gitian.sigs, etc
1cp bitcoin/contrib/gitian-build.py .
2# add MacOS stuff 
3python3 gitian-build.py --setup sjors -c master
4python3 gitian-build.py --detach-sign --no-commit -b sjors -c master

This throws an error for me, see #13623. The original bash script also throws that error, so probably not related. Just means I can’t test the rest of the process on this VM.

When running --setup --docker it throws an error:

0ERRO[0000] failed to dial gRPC: cannot connect to the Docker daemon. Is 'docker daemon' running on this host?: dial unix /var/run/docker.sock: connect: permission denied

docker ps also throws a permission error; sudo docker ps works. sudo usermod -aG docker $USER solves the problem. It creates an image base-bionic-amd64.

Trying to build with Docker complains:

0Compiling master Linux
1--- Building for bionic amd64 ---
2Stopping target if it is up
3Making a new image copy
4Error: No such container: gitian-target

The bash script throws the same complaint, so that’s not related.

When running gitian-build.py without arguments, it should probably just return --help (or otherwise point out that this exists).

Is there a corresponding PR to update the instructions? (mostly just renaming .sh to .py)

Can you add make to the dependencies installed during --setup?

For another PR: having a config flag to get a Trusty base image can be useful when building backports.

ken2812221 commented at 5:37 pm on July 17, 2018: contributor

Compiling master Linux — Building for bionic amd64 — Stopping target if it is up Making a new image copy Error: No such container: gitian-target

I believe the docker problem is already exist on gitian-build.sh because I saw the same error trying to use gitian-build.sh to create docker image.

Can you add make to the dependencies installed during –setup?

Why?

Sjors commented at 5:53 pm on July 17, 2018: member

Correct, it happens both with bash and python.

Without “make” setup throws an error on a fresh Bionic machine (unless you install all dependencies recommended in the docs, which is overkill).

MarcoFalke commented at 6:02 pm on July 17, 2018: member

utACK 78f06e4af76b6e4550b8eed5a521684140a4

MarcoFalke merged this on Jul 17, 2018

MarcoFalke closed this on Jul 17, 2018

MarcoFalke referenced this in commit 9cdb19fe67 on Jul 17, 2018

ken2812221 deleted the branch on Jul 17, 2018

Sjors commented at 6:35 pm on July 17, 2018: member

On Debian 9, when trying to --setup with --docker, I’m getting Package 'docker.io' has no installation candidate.

So I installed Docker manually, but get the same error. Disabling programs += ['docker.io'] does the trick.

hebasto referenced this in commit 2608860ace on Aug 17, 2018

hebasto referenced this in commit ad6db7f8c4 on Aug 17, 2018

hebasto referenced this in commit a2a316e265 on Aug 19, 2018

hebasto referenced this in commit a2b1d10189 on Sep 5, 2018

hebasto referenced this in commit bc4ca468a0 on Sep 5, 2018

hebasto referenced this in commit be88ede292 on Sep 6, 2018

UdjinM6 referenced this in commit 3313bbd515 on Oct 21, 2018

MarcoFalke referenced this in commit f49b8d4783 on May 20, 2019

sidhujag referenced this in commit 19c473999e on May 20, 2019

CryptoCentric referenced this in commit 2b0edc5565 on May 21, 2019

CryptoCentric referenced this in commit 2a5ffd9808 on May 4, 2021

CryptoCentric referenced this in commit b1d54c1ac3 on May 4, 2021

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

Migrate gitian-build.sh to python #13623