-
Notifications
You must be signed in to change notification settings - Fork 38.7k
Avoid concurrency issue when make multiple target #13465
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
Conversation
|
From make: Special Targets: .NOTPARALLEL
|
|
Will this make the entire build non-parallel? If so, that would be bad. |
As @fanquake posted
The code building targets are still can run in parallel because they are in src/Makefile, this change only affect the target in the top Makefile. |
|
ok utACK from me in that case |
|
If there's a race condition, something's broken. We should clarify the dependencies rather than serializing. Any idea where things break down? |
|
Agree with @theuni, looks like this avoids a problem instead of fixing it. |
|
I think that this should be enough to fix it. Untested: diff --git a/Makefile.am b/Makefile.am
index 8a8debb..3a725d3 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -95,7 +95,7 @@ $(OSX_APP)/Contents/Resources/bitcoin.icns: $(OSX_INSTALLER_ICONS)
$(MKDIR_P) $(@D)
$(INSTALL_DATA) $< $@
-$(OSX_APP)/Contents/MacOS/Bitcoin-Qt: $(BITCOIN_QT_BIN)
+$(OSX_APP)/Contents/MacOS/Bitcoin-Qt: all-recursive
$(MKDIR_P) $(@D)
STRIPPROG="$(STRIP)" $(INSTALL_STRIP_PROGRAM) $< $@
I believe the issue is that the top-level calls a sub-make for all, as well as for src/qt/bitcoin-qt, and those end up racing. Solve that by having deploy depend on the sub-make already being complete, which also matches the way the Windows dependencies are handled. As a side-effect, this makes |
|
@theuni Fixed |
|
utACK cf01fd6, makes sense! |
|
@ken2812221 Thanks! Tested locally. I can easily reproduce the racy build failure in master, and this appears to fix it. |
cf01fd6 Avoid concurrency issue (Chun Kuan Lee) Pull request description: From bitcoin#13406, changed travis job target for Mac to `all deploy`, but this could cause a race condition. Simply add `.NOTPARALLEL` to avoid it. Related jobs: https://travis-ci.org/bitcoin/bitcoin/jobs/391863281 https://travis-ci.org/bitcoin/bitcoin/jobs/391907335 Close bitcoin#13469 Tree-SHA512: 75c6585fe770dc70e6256dcdf97af37274f95a9240ed5a5cea2ca92b8411893b80327335587270351b128f56cb2e00f684db7c19b1602048132b734dad6ececa
cf01fd6 Avoid concurrency issue (Chun Kuan Lee) Pull request description: From bitcoin#13406, changed travis job target for Mac to `all deploy`, but this could cause a race condition. Simply add `.NOTPARALLEL` to avoid it. Related jobs: https://travis-ci.org/bitcoin/bitcoin/jobs/391863281 https://travis-ci.org/bitcoin/bitcoin/jobs/391907335 Close bitcoin#13469 Tree-SHA512: 75c6585fe770dc70e6256dcdf97af37274f95a9240ed5a5cea2ca92b8411893b80327335587270351b128f56cb2e00f684db7c19b1602048132b734dad6ececa
From #13406, changed travis job target for Mac to
all deploy, but this could cause a race condition.Simply add
.NOTPARALLELto avoid it.Related jobs:
https://travis-ci.org/bitcoin/bitcoin/jobs/391863281
https://travis-ci.org/bitcoin/bitcoin/jobs/391907335
Close #13469