-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Docs4Doge: update of gitian builder script + documentation (follow #2396) #2579
Docs4Doge: update of gitian builder script + documentation (follow #2396) #2579
Conversation
Fix issues of Dogecoin folder getting old files and never cleaned
External signing enabled. What would be the file to check to verify hashes of |
Just added |
I think the main thing is this needs devrandom/gitian-builder#253 to be merged, and that doesn't seem to be getting attention in the original repo. I suspect because you've not really explained why it's needed, and the impact of disabling the cache. |
The maintainer should be aware of the PR#253. It supersedes a @micaelmalta PR#249 where devrandom (and other) reviewed but wanted some change, I implemented it. Another pending PR was waiting on this feature and, while pushing change on @micaelmalta fork, devrandom told me to do it upstream. So it's noticed, they were reactive, but I've no more reply since I did the PR. Plus, modifications became slightly trickier. I was waiting on some review/tests, mainly for LXC & KVM. I will ask to see what they want to do and to clarify the situation. If anyone can test this script with lxc & kvm, with and without |
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.
- The change in descriptor names from
signer
tosigned
is undocumented and incompatible with the current state of 1.21 - why is this needed? - Since so much is getting lost in the documentation, how would you feel about creating a new document, call it
simple-gitian-building.md
and then once it's stable and complete, we can remove the old doc later? We can, if proven to work, update links to go to the simple guide instead.
Further questions and suggestions are inside.
Get a response from devrandom, gitian-builder repository is going in maintenance mode because bitcoin is switching to guix build process. Did you already plan to switch to guix with 1.21 ? |
Not to my knowledge. I'd say we'd need to run both in parallel for a while. We can always fork gitian-builder and maintain what we need until we no longer need it. Can we build something on the CI without removing apt-cacher? yes I think so. Question to you: what's the impact of not removing it to the success of this PR? |
Or a patch ? If it needs only this change, imo the fork doesn't worth it. At first, would you like to keep this feature to enable gitian without apt-cacher ? Easy to remove, but may be great to facilitate signing, may be interesting if we keep it for a while. |
Maybe we should break it up into 2 steps:
|
I undo this change, I go with signer. It was not introduced by me, but it looks like it was done to make executables names consistent with gitian descriptor. I fixed comments, using only We can't build signed binaries now, right ? It seems the script is expecting a specific tag release on https://github.com/dogecoin/dogecoin-detached-sigs. I saw some comments where you were working on signatures. I'm going to submit a patch for devrandom/gitian-builder repo, to disable |
Yes, I think signer is good because that's what it has been since forever? I'd just edit the commit, I'll anyway request a squash on the first 2 commits, so may as well just rename back and
We can. We're just not doing it. At least for Windows. For macOS we're doing it but with the new Apple requirements it's a bit more complicated than what we have right now.
Correct. I went into a bit of a rabbit hole to also figure out how to do the signed submissions to Apple to get rid of the security warning there. I hope to have a complete picture of what needs to get done over the next week, otherwise we should first fix Windows and then worry about macOS. |
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.
Did the second run focusing mostly on grammar and guide. I have 2 non-grammar things in here:
- I propose to remove KVM support from this script if this is not documented
LXC_BRIDGE
needs a solution
if [[ $lxc = true ]] | ||
then | ||
export USE_LXC=1 | ||
export LXC_BRIDGE=br0 |
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.
IMPORTANT: This should still be set for LXC build to work, recommend doing that above when processing --lxc
, but do an optional setting, so that it can be overridden (on systems where I have multiple virtualization solutions, this is nearly never br0
). Alternatively, document it in the guide that this must ALWAYS be set or add 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.
Do you want to go with something like that in option parsing :
--lxc)
USE_LXC=1
export LXC_BRIDGE=${LXC_BRIDGE:-br0}
;;
Defining br0
as default value, and letting the user override it if needed ?
echo "Using ${proc} CPU and ${mem} RAM" | ||
|
||
# Control the selection of a single virtualisation software | ||
if [ $(($USE_LXC + $USE_DOCKER)) -ge 2 ]; then |
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.
Since KVM is not documented, could we please do an elif
with -lt 1
and throw an error too for now?
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.
Let's see your answer related to keep or remove KVM. In anticipation of the case where you want to remove it, do you want to use docker or lxc as default, or do we require from the user to specify it ?
|
||
# Control the selection of a single virtualisation software | ||
if [ $(($USE_LXC + $USE_DOCKER)) -ge 2 ]; then | ||
echo "$scriptName: Specify a single virtualisation solution between Docker, LXC or KVM." |
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.
echo "$scriptName: Specify a single virtualisation solution between Docker, LXC or KVM." | |
echo "$scriptName: Specify a single virtualisation solution between Docker or LXC." |
Co-authored-by: Patrick Lodder <[email protected]>
Co-authored-by: Patrick Lodder <[email protected]>
I'm not sure what should be strategy for KVM support. You suggest removing references within the script too (your current suggestions are cleaning it for the documentation, not the script) ? It's possibly not so far from working, I wasn't able to use LXC & KVM to test it (mysterious error where I did no investigated too long, never used this tools), but I hope it's designed to (almost) work for someone who knows it. What I had in mind is to keep KVM support as incomplete and state it as incomplete in the expectation someone will try & fix it. It has less opportunity to happen if we remove all content related to kvm. From Karl Fogel - Producing Open Source Software How to Run a Successful Free Software Project :
Are you sure you want to remove KVM (personally, I'm in favor of keeping it) ? |
I'm okay with keeping it but could you then please document how far you got with it? What errors did you run into?
As for LXC, the gitian-builder side definitely works because since #2501 I build it, so it's on my todo to test end-to-end with this script and we'll get that done. |
KVMFirst, on debian the installation of kvm required to find manually package, I possibly used the wrong one. I used python-vm-builder 0.12.4, python-cheetah 2.4.4, ubuntu-keyring 2018.09.18.1(?). When building the base vm for bionic, it's throwing the following error:
I tried with
LXCI'm able to build base image, to start abcsxyz:~$ ./gitian-build.sh --setup --build --lxc -o l 1.14.5
pdating apt-get repository (log in var/install.log)
Installing additional packages (log in var/install.log)
Traceback (most recent call last):
6: from ./bin/gbuild:334:in `<main>'
5: from ./bin/gbuild:334:in `each'
4: from ./bin/gbuild:336:in `block in <main>'
3: from ./bin/gbuild:336:in `each'
2: from ./bin/gbuild:341:in `block (2 levels) in <main>'
1: from ./bin/gbuild:120:in `build_one_configuration'
./bin/gbuild:23:in `system!': failed to run on-target -u root -e DEBIAN_FRONTEND=noninteractive apt-get --no-install-recommends -y install curl g++-aarch64-linux-gnu g++-7-aarch64-linux-gnu gcc-7-aarch64-linux-gnu binutils-aarch64-linux-gnu g++-arm-linux-gnueabihf g++-7-arm-linux-gnueabihf gcc-7-arm-linux-gnueabihf binutils-arm-linux-gnueabihf g++-7-multilib gcc-7-multilib binutils-gold git-core pkg-config autoconf libtool automake faketime bison bsdmainutils ca-certificates python3 >> var/install.log 2>&1 (RuntimeError)
For both LXC & KVM, it seems mainly configuration issues for me. I've the good hope that people who understand those tools are able to use the script 😅 |
I've got this to work with LXC (and with apt-cache), but Looking at the issues we're running into, perhaps we should pin to a gitian-builder commit in the script by default. I'll propose that separately after we merge this one. |
The PR on |
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 now works as documented (per the updated documentation) with docker for me. All that is needed is a squash on the commits: would prefer to have Micael's commits squashed into one, and the followup commits into another, with an end result of a neat 2 commits.
I'll take care of the LXC documentation in a separate PR.
Hi, can you do it yourself ? I deleted my fork, as I do not expect to work anymore on Dogecoin, just leaving my work/PRs to be reused. |
Even better, I can do it for you. Am sad to hear that, but thank you for everything you have done! ❤️ |
Merged with #2973! Many thanks! ❤️ |
Related to Docs4Doge initiative, based on & following PR #2396.
Following @micaelmalta work, I reused a good part of his script and edited
gitian-builder.sh
to fit in a single script, like it was. This is mainly an update of contrib/gitian-build.sh and his documentation doc/gitian-building.md (see it to try the script).A lot of deleted files are coming from previous documentation images.
It's having Docker available for sure, and maybe LXC & KVM aren't so far if you're able to use/configure them (with some tests I weren't).
Pending PRs on devrandom/gitian-builder
It relies on 2 PR on devrandom/gitian-builder repository. devrandom/gitian-builder#254, already merged, and devrandom/gitian-builder#253 to get
--disable-apt-cacher
option.There is some interest in the PR, I suppose it will be merged before we finish to work on this.
For now, to try the script with those features, you can replace inside
gitian-build.sh
the line withby a branch of my gitian-builder repository :
Improvement in mind
It could be used like that, but some possible improvement are also possible before merging. I think I will investigate about the following from @patricklodder comment :
+ Possibility to configure apt-cacher (@patricklodder may apt-cacher be useful for CI or something else, even personally ?) ?
P.S. : Sorry for parallel PR with #2396, I had the choice between PR @micaelmalta's repo to include my changes or create a new PR to Dogecoin Core repo. Thought it was more appropriate to do it that way.