Skip to content
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: Improve error handling #15549

Merged
merged 1 commit into from Mar 9, 2019

Conversation

Projects
None yet
7 participants
@laanwj
Copy link
Member

commented Mar 6, 2019

Improve error handling in gitian builds:

  • Set fail-on-error and pipefail flag, this causes a command to fail when either of the pipe stages fails, not only when the last of the stages fails, so this improves error detection.
  • Also use xargs instead of find -exec, because find will not propagate errors in the executed command, but xargs will.

This will avoid some issues like #15541 where non-determinism is silently introduced due to errors caused by environment conditions (such as lack of disk space in that case).

@laanwj laanwj added this to the 0.18.0 milestone Mar 6, 2019

@laanwj laanwj force-pushed the laanwj:2019_03_gitian_error_handling branch from f05c1e4 to 9793a5c Mar 6, 2019

@hebasto

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

Concept ACK.

@laanwj laanwj requested a review from theuni Mar 6, 2019

@practicalswift

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

Concept ACK

@laanwj

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2019

#15541 (comment) :

I see the default size for the LXC disk is 10240MB maybe that's no longer enough

Independently of this we should probably increase the default LXC base VM image size (here https://github.com/devrandom/gitian-builder/blob/master/bin/make-base-vm#L267 ), or make the descriptor clean up the intermediate result after each host build. I think I have a slight preference for the latter as it doesn't require everyone to regenerate—or resize—their base images.

@luke-jr

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15554 (docs: binary tar improvement by cisba)

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.

@@ -1,5 +1,5 @@
#!/bin/sh

set -e -o pipefail

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Mar 7, 2019

Member

DrahtBot fails with

../contrib/devtools/split-debug.sh: 2: set: Illegal option -o pipefail

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Mar 7, 2019

Member

On Ubuntu the default shell is dash (aka Debian Almquist Shell), to which /bin/sh is symlink. When your shell script is run with #!/bin/sh, you are effectively trying to run it with the default shell. However, dash doesn't have the pipefail option, which is why you're getting the error.

https://askubuntu.com/a/886540

This comment has been minimized.

Copy link
@laanwj

laanwj Mar 7, 2019

Author Member

Oh crap. I hate shellscript programming so much.

This comment has been minimized.

Copy link
@laanwj

laanwj Mar 7, 2019

Author Member

Oh wait, is this only about split-debug.sh.in, not the main gitian scripts? I don't think the pipefail matters there at all as the script doesn't use pipes.

Edit: removed it here, please retry

@laanwj laanwj closed this Mar 7, 2019

@laanwj laanwj reopened this Mar 7, 2019

@laanwj laanwj force-pushed the laanwj:2019_03_gitian_error_handling branch from 9793a5c to 32da92b Mar 7, 2019

@bitcoin bitcoin deleted a comment from DrahtBot Mar 7, 2019

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

Gitian builds for commit d211edb (master):

Gitian builds for commit 6eb1a44 (master and this pull):

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

utACK 32da92b

laanwj added a commit that referenced this pull request Mar 9, 2019

gitian: Improve error handling
Github-Pull: #15549
Rebased-From: 32da92b
Tree-SHA512: c79455ed3aa0e529821ed30c5d36244dcc9fbf9154b63dfcce4789143d8a737f83ea2aaea8993748babc4cec45ae0e88449d2a6ddc90dcf62a39cf8d6a2ace2e

@laanwj laanwj merged commit 32da92b into bitcoin:master Mar 9, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Mar 9, 2019

Merge #15549: gitian: Improve error handling
32da92b gitian: Improve error handling (Wladimir J. van der Laan)

Pull request description:

  Improve error handling in gitian builds:

  - Set fail-on-error and pipefail flag, this causes a command to fail when either of the pipe stages fails, not only when the last of the stages fails, so this improves error detection.
  - Also use `xargs` instead of `find -exec`, because `find` will not propagate errors in the executed command, but `xargs` will.

  This will avoid some issues like #15541 where non-determinism is silently introduced due to errors caused by environment conditions (such as lack of disk space in that case).

Tree-SHA512: d5d3f22ce2d04a75e5c25e935744327c3adc704c2d303133f2918113573a564dff3d3243d5569a2b93ee7eb0e97f8e1b1ba81767e966af9015ea711a14091035

@fanquake fanquake removed the Needs backport label Mar 9, 2019

@fanquake

This comment has been minimized.

Copy link
Member

commented Mar 9, 2019

This has been backported to 0.18.0 in f810f14.

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Mar 10, 2019

gitian: Improve error handling
Github-Pull: bitcoin#15549
Rebased-From: 32da92b
Tree-SHA512: c79455ed3aa0e529821ed30c5d36244dcc9fbf9154b63dfcce4789143d8a737f83ea2aaea8993748babc4cec45ae0e88449d2a6ddc90dcf62a39cf8d6a2ace2e
@hebasto

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

Just noted that -e option is already set in bin/gbuild:

 script.puts "set -e"

So set -o pipefail (without -e) could be sufficient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.