/bitcoin.org

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Build: Push Function Include Down Into Sub-Shell #944

Merged
merged 1 commit into from Jul 6, 2015

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@harding @saivann
@harding
Contributor

harding commented Jul 5, 2015

Fixes issue (bug) #941

While installing rvm as root (which is a pain) for a test environment, I realized what the problem with the current script was:

  • kill 0 kills all process in the current process group. That includes children of the current shell, but not the shell itself (which has the pgid of its parent shell).
  • By sourcing rvm.sh we were including the rvm shell function into the shell. Unlike commands such as git, rsync, etc..., rvm wasn't a child process.
  • So, when kill 0 was run, it had no effect on the rvm function.

The solution was easy: push the sourcing of the rvm.sh function down a level into the build process sub-shell.

This PR has been implemented and well tested on the build server (and implemented and lightly tested on the translation preview server). The testing on the build server included:

  • A regular build. Rsync was run and the tmp directory in the /tmp directory was removed as expected.
  • A build with an artificially-induced error. The build aborted and the tmp directory was removed. Rsync was not run.
  • Two successive builds: when the second build started, the first build aborted and its tmp directory was removed. When the second build finished, rsync was run and the tmp directory was removed.

This PR also now prints a message on success, as the shell-generated "Terminated" message after a successful run confused me for a bit.

I think this is all I'm going to do for now. I have an idea for queuing of builds using lock files, but now that the main problem is fixed, I find I'm not so enthusiastic about implementing it. :-)

As noted, the solution is already deployed, so I think this PR can be merged as soon as @saivann has reviewed it. (It can be merged using the GitHub merge button since it doesn't affect website content.)
@harding
Build: Push Function Include Down Into Sub-Shell 
Push RVM function include down into subshell to allow build scripts to
kill all sub-processes.

[skip ci]
Closes #941
This commit was signed with a verified signature.
@harding harding David A. Harding
GPG key ID: 4B29C30FF29EC4B7 Learn about signing commits
1939eb1

@saivann saivann merged commit 1939eb1 into bitcoin-dot-org:master Jul 6, 2015

@saivann
Contributor

saivann commented Jul 6, 2015

@harding Ah, that makes sense. I've tested the script locally and it works perfectly. Thanks a lot for completing my investigation (and for updating the translation preview script on GitHub and on the VM as well)!!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment