-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Gitian unit e #511
Gitian unit e #511
Conversation
Signed-off-by: Mateusz Morusiewicz <[email protected]>
Signed-off-by: Mateusz Morusiewicz <[email protected]>
Signed-off-by: Mateusz Morusiewicz <[email protected]>
Signed-off-by: Mateusz Morusiewicz <[email protected]>
Signed-off-by: Mateusz Morusiewicz <[email protected]>
Signed-off-by: Mateusz Morusiewicz <[email protected]>
Signed-off-by: Mateusz Morusiewicz <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generally looks good. It corresponds to the earlier version I have tested. I didn't test this pull request by running builds, but I think it's good enough to be merged and we can build on it with more changes. Especially as the current version in the repo doesn't work for unit-e it would be good to get to a start of a working version.
So utACK from me.
Signed-off-by: Mateusz Morusiewicz <[email protected]>
Signed-off-by: Mateusz Morusiewicz <[email protected]>
@@ -17,30 +17,30 @@ | |||
import sys | |||
import os | |||
|
|||
# Debian 6.0.9 (Squeeze) has: | |||
# Debian 8.11 (jessie) has: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am completely fine with the bump of version requirements. ACK this one in particular.
if args.kvm: | ||
programs += ['python-vm-builder', 'qemu-kvm', 'qemu-utils'] | ||
programs += ['apt-cacher-ng', 'python-vm-builder', 'qemu-kvm', 'qemu-utils'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does apt-cacher-ng
do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's required for lxc and kvm, but not for docker (hence the change, notice it was removed from the always-installed stuff).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But what does it do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It creates a local cache for apt from within the containers (makes it work way faster). I'd leave it be.
@@ -28,14 +28,14 @@ packages: | |||
- "ca-certificates" | |||
- "python" | |||
remotes: | |||
- "url": "https://github.com/unite/unite.git" | |||
"dir": "unite" | |||
- "url": "[email protected]:dtr-org/unit-e.git" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe these should be added as a special case to the clone machine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need it as we will be public soon anyway (and then we will use the https), and this url is at the moment overwritten in the script anyway (it's there in case you want to run gitian build manually).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have a way of just retaining some documents verbatim in the clone machine, called "appropriated files" ( https://github.com/dtr-org/clonemachine/blob/master/fork.py#L69 ). Maybe the gitian build script should be part of these?
@cornelius WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think we should "appropriate" the gitian build script. It still might make sense to cover the GitHub URL as special case. I think I have seen this handled wrongly in clonemachine before in other places as well.
files: [] | ||
script: | | ||
|
||
WRAP_DIR=$HOME/wrapped | ||
HOSTS="i686-pc-linux-gnu x86_64-linux-gnu arm-linux-gnueabihf aarch64-linux-gnu" | ||
CONFIGFLAGS="--enable-glibc-back-compat --enable-reduce-exports --disable-bench --disable-gui-tests" | ||
CONFIGFLAGS="--enable-glibc-back-compat --enable-reduce-exports --disable-bench --disable-gui-tests --with-gui=no" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
completely ACK this change :-)
@@ -72,7 +72,7 @@ script: | | |||
|
|||
function create_per-host_linker_wrapper { | |||
# This is only needed for trusty, as the mingw linker leaks a few bytes of | |||
# heap, causing non-determinism. See discussion in https://github.com/unite/unite/pull/6900 | |||
# heap, causing non-determinism. See discussion in https://github.com/bitcoin/bitcoin/pull/6900 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also something which might be worthwhile to be added to the clonemachine as an exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is the kind of case I was thinking about. I'll create an issue for clonemachine to keep track of this.
import platform | ||
|
||
def install_linux_deps(): | ||
global args |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that it was like that before, but I think it's a bad idea to use global
here, when args
could be passed as a param from the function install_deps
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually like the args global, as it'd have to be passed to every function.
Also, this PR is mostly about making the script work for Unit-e, we have more refactoring coming up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a patch in the works to get rid of the global variables. Will be in a separate pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for the record, there are plenty of reasons why using global
is usually a bad idea:
- It's more difficult to know where these variables are accessed and modified.
- It's easier to introduce subtle bugs because we could have unexpected read/write accesses to some values.
- It's more difficult to reason about types
- It's more difficult to reason about an object's evolution.
- The IDEs have similar problems, so their introspection capabilities become diminished.
- It's more difficult to do proper unit testing (I know that this does not apply here because testing is difficult for other reasons too) as everything becomes entangled and side-effects are ubiquitous.
- It's more difficult to refactor code.
- And there's an extra line that could be avoided by just passing a parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 5ee0ce7
I like the version bumps, and as we discussed, I do think they are completely appropriate. We might want to add some things to the clone machine, but that's outside the scope of this pull request.
I do lack the time to give that some thorough testing, which I would have liked to do simply to build up the experience with gitian. Some other time :-)
Still to do:
Addresses #466