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

build: Finish up out-of-tree changes #8133

Merged
merged 7 commits into from Jun 10, 2016

Conversation

Projects
None yet
7 participants
@theuni
Member

theuni commented Jun 1, 2016

This fixes the remaining out-of-tree issues:

  • The entire leveldb dir was copied out
  • Python caches were left hanging around
  • A qt file was left dangling in the build-dir
  • The rpc tests couldn't be run
  • OSX's make deploy was broken

make distcheck now passes, and Travis has been switched to an out-of-tree build for dogfooding purposes. Fingers-crossed that it works.

theuni added some commits Jun 1, 2016

build: out-of-tree fixups
Don't glob the leveldb for dist. That means we need to enumerate the headers.
build: more out-of-tree fixups
- clear the __pycache__ during 'make clean'
- Copy the qrc locale file to a temp location and remove it when finished
  (rcc expects everything to be in the same path)
build: a few ugly hacks to get the rpc tests working out-of-tree
- Link pull-tester/rpc-tests.py to the build dir
- Add the build-dir's config to the python path so that tests can find it
- The tests themselves are in srcdir
- Clean up __pycache__ in 'make clean'
build: fix out-of-tree 'make deploy' for osx
The plist is generated, lives in builddir.
@@ -37,7 +38,7 @@
# terminal via ANSI escape sequences:
BOLD = ('\033[0m', '\033[1m')
RPC_TESTS_DIR = BUILDDIR + '/qa/rpc-tests/'
RPC_TESTS_DIR = SRCDIR + '/qa/rpc-tests/'

This comment has been minimized.

@MarcoFalke

MarcoFalke Jun 2, 2016

Member

Note to myself: Does the cache (from create_cache.py et al.) end up in SRCDIR now?

@MarcoFalke

MarcoFalke Jun 2, 2016

Member

Note to myself: Does the cache (from create_cache.py et al.) end up in SRCDIR now?

This comment has been minimized.

@laanwj

laanwj Jun 3, 2016

Member

Wasn't it always (and still is) the case that the cache ends up in the current directory when the tests are launched?

@laanwj

laanwj Jun 3, 2016

Member

Wasn't it always (and still is) the case that the cache ends up in the current directory when the tests are launched?

This comment has been minimized.

@MarcoFalke

MarcoFalke Jun 3, 2016

Member

Correct.

Though, this should be changed. Having different caches lying around in folders that happen to be your working dir right then is misleading at best.

Anyway, this is unrelated to this pull.

@MarcoFalke

MarcoFalke Jun 3, 2016

Member

Correct.

Though, this should be changed. Having different caches lying around in folders that happen to be your working dir right then is misleading at best.

Anyway, this is unrelated to this pull.

This comment has been minimized.

@theuni

theuni Jun 3, 2016

Member

I'm having a hard time answering that question because of the symlink interaction with Python. builddir/rpc-tests.py is now a symlink to sourcedir/rpc-tests.py because it needs to be writable. But that makes the definition of "current dir" hazy.

@MarcoFalke I suppose it would be best to make the location explicit.

@theuni

theuni Jun 3, 2016

Member

I'm having a hard time answering that question because of the symlink interaction with Python. builddir/rpc-tests.py is now a symlink to sourcedir/rpc-tests.py because it needs to be writable. But that makes the definition of "current dir" hazy.

@MarcoFalke I suppose it would be best to make the location explicit.

This comment has been minimized.

@MarcoFalke

MarcoFalke Jun 3, 2016

Member

Sure, this is on my list of cleanups. If no one beats me to it, you can expect it some time after feature freeze. ;)

@MarcoFalke

MarcoFalke Jun 3, 2016

Member

Sure, this is on my list of cleanups. If no one beats me to it, you can expect it some time after feature freeze. ;)

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jun 2, 2016

Member

Concept ACK

Member

MarcoFalke commented Jun 2, 2016

Concept ACK

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Jun 2, 2016

Member

Generally speaking, if it's used as-is from git, it's srcdir. If it's generated or pre-processed, it's builddir.

Put another way: after checkout, think of srcdir as read-only. If anything is modified, it must happen in builddir.

Member

theuni commented Jun 2, 2016

Generally speaking, if it's used as-is from git, it's srcdir. If it's generated or pre-processed, it's builddir.

Put another way: after checkout, think of srcdir as read-only. If anything is modified, it must happen in builddir.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 3, 2016

Member

utACK 142ffc7

Member

laanwj commented Jun 3, 2016

utACK 142ffc7

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jun 3, 2016

Member

make check fails with:

Running test/bitcoin-util-test.py...
Traceback (most recent call last):
  File "../../src/test/bitcoin-util-test.py", line 8, in <module>
    import buildenv
ImportError: bad magic number in 'buildenv': b'\x03\xf3\r\n'
Member

MarcoFalke commented Jun 3, 2016

make check fails with:

Running test/bitcoin-util-test.py...
Traceback (most recent call last):
  File "../../src/test/bitcoin-util-test.py", line 8, in <module>
    import buildenv
ImportError: bad magic number in 'buildenv': b'\x03\xf3\r\n'
@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Jun 3, 2016

Member

@MarcoFalke Yes, I just saw that as well while trying to answer the question above. I'm not sure why Travis passes.

I'll fix that one up.

Member

theuni commented Jun 3, 2016

@MarcoFalke Yes, I just saw that as well while trying to answer the question above. I'm not sure why Travis passes.

I'll fix that one up.

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Jun 3, 2016

Member

Pushed a quick fix for @MarcoFalke's problem. I'm not sure if we really need that or not, but it's probably better than each dev running into the problem and googling for themselves.

The issue is this: previous invocations of python2 in an in-tree build would leave a buildenv.pyc hanging around in srcdir. We prepend python's search path with builddir, but for some reason it ends up finding the srcdir/buildenv.pyc (and missing .py) before the builddir/buildenv.py. python3 refuses to load the old python2 .pyc, and errors because it can't find a .py at the same path for recompilation.

With python3, pyc's are organized into a pycache dir, and tagged like: pycache/buildenv.cpython-35.pyc. So it should not be possible to hit this case anymore.

Member

theuni commented Jun 3, 2016

Pushed a quick fix for @MarcoFalke's problem. I'm not sure if we really need that or not, but it's probably better than each dev running into the problem and googling for themselves.

The issue is this: previous invocations of python2 in an in-tree build would leave a buildenv.pyc hanging around in srcdir. We prepend python's search path with builddir, but for some reason it ends up finding the srcdir/buildenv.pyc (and missing .py) before the builddir/buildenv.py. python3 refuses to load the old python2 .pyc, and errors because it can't find a .py at the same path for recompilation.

With python3, pyc's are organized into a pycache dir, and tagged like: pycache/buildenv.cpython-35.pyc. So it should not be possible to hit this case anymore.

build: add temporary fix for "bad magic number" error in out-of-tree …
…builds

This was caused by an pyc files hanging around from previous
python2 invocations, when the matching .py missing from that path.

This should not be a problem with python3's tagged caches.
@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jun 3, 2016

Member

Tested ACK 340012d

Member

MarcoFalke commented Jun 3, 2016

Tested ACK 340012d

@@ -29,6 +29,7 @@
import tempfile
import re
sys.path.append("qa/pull-tester/")

This comment has been minimized.

@MarcoFalke

MarcoFalke Jun 3, 2016

Member

Is it safe to assume everyone runs the tests from the root of buildir?

@MarcoFalke

MarcoFalke Jun 3, 2016

Member

Is it safe to assume everyone runs the tests from the root of buildir?

This comment has been minimized.

@luke-jr

luke-jr Jun 3, 2016

Member

Probably not, but FWIW at least I do...

@luke-jr

luke-jr Jun 3, 2016

Member

Probably not, but FWIW at least I do...

This comment has been minimized.

@btcdrak

btcdrak Jun 3, 2016

Member

I do too.

@btcdrak

btcdrak Jun 3, 2016

Member

I do too.

This comment has been minimized.

@sipa

sipa Jun 3, 2016

Member

Heh, I run them from qa/pull-tester. I had no idea you could run them from the root...

@sipa

sipa Jun 3, 2016

Member

Heh, I run them from qa/pull-tester. I had no idea you could run them from the root...

This comment has been minimized.

@MarcoFalke

MarcoFalke Jun 4, 2016

Member

Not sure how to fix this...

What about byte-compiling the rpc-tests.py instead of linking?

@MarcoFalke

MarcoFalke Jun 4, 2016

Member

Not sure how to fix this...

What about byte-compiling the rpc-tests.py instead of linking?

This comment has been minimized.

@laanwj

laanwj Jun 13, 2016

Member

Heh, I run them from qa/pull-tester. I had no idea you could run them from the root...

Same here. But running from the root is ok with me.

@laanwj

laanwj Jun 13, 2016

Member

Heh, I run them from qa/pull-tester. I had no idea you could run them from the root...

Same here. But running from the root is ok with me.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jun 5, 2016

Member

Extensively-tested, with the following results:

I built, make check'd, and confirmed the reported version numbers for:

  1. Before this PR (plus #7522), built in a clean git clone: v0.12.99.0-fd89d8e
  2. Before this PR (plus #7522), built in a clean dist dir: v0.12.99.0-gfd89d8e
  3. Before this PR (plus #7522), built in a clean dist dir under a foreign git checkout: v0.12.99.0-6862d12
  4. With this PR (plus #7522), built in a clean git clone: v0.12.99.0-08cb06e
  5. With this PR (plus #7522), built in a subdirectory of a clean git clone owned by another user: v0.12.99.0-08cb06e-dirty
  6. With this PR (plus #7522), built in a parallel directory of a clean git clone owned by another user, under a foreign git checkout: v0.12.99.0-08cb06e-dirty
  7. With this PR (plus #7522), built in a clean dist dir under a foreign git checkout: v0.12.99.0-6862d12
  8. With this PR (plus #7522), built in a clean dist dir: v0.12.99.0-g08cb06e
  9. With this PR (plus #7522), built in a subdirectory of a clean dist dir owned by another user: v0.12.99.0-g08cb06e
  10. With this PR (plus #7522), built in a parallel directory of a clean dist dir owned by another user: v0.12.99.0-g08cb06e

Note that:

  • 08cb06e = With this PR (plus #7522)
  • fd89d8e = Before this PR (plus #7522)
  • 6862d12 = Commit in empty foreign git repo

Observations:

  1. No change to previously-supported build scenarios.
  2. Dist dir builds within a foreign git repository incorrectly use the foreign git repo's commit hash in the version. This is already a problem (even with #7522 apparently!), so not a blocker for this PR.
  3. Dist dir builds prefix the commit hash with 'g', while git builds do not. This is already a problem, so also not a regression in this PR.
  4. Clean git builds using a different build dir incorrectly tag the version as "-dirty". This should probably be fixed in this PR.
Member

luke-jr commented Jun 5, 2016

Extensively-tested, with the following results:

I built, make check'd, and confirmed the reported version numbers for:

  1. Before this PR (plus #7522), built in a clean git clone: v0.12.99.0-fd89d8e
  2. Before this PR (plus #7522), built in a clean dist dir: v0.12.99.0-gfd89d8e
  3. Before this PR (plus #7522), built in a clean dist dir under a foreign git checkout: v0.12.99.0-6862d12
  4. With this PR (plus #7522), built in a clean git clone: v0.12.99.0-08cb06e
  5. With this PR (plus #7522), built in a subdirectory of a clean git clone owned by another user: v0.12.99.0-08cb06e-dirty
  6. With this PR (plus #7522), built in a parallel directory of a clean git clone owned by another user, under a foreign git checkout: v0.12.99.0-08cb06e-dirty
  7. With this PR (plus #7522), built in a clean dist dir under a foreign git checkout: v0.12.99.0-6862d12
  8. With this PR (plus #7522), built in a clean dist dir: v0.12.99.0-g08cb06e
  9. With this PR (plus #7522), built in a subdirectory of a clean dist dir owned by another user: v0.12.99.0-g08cb06e
  10. With this PR (plus #7522), built in a parallel directory of a clean dist dir owned by another user: v0.12.99.0-g08cb06e

Note that:

  • 08cb06e = With this PR (plus #7522)
  • fd89d8e = Before this PR (plus #7522)
  • 6862d12 = Commit in empty foreign git repo

Observations:

  1. No change to previously-supported build scenarios.
  2. Dist dir builds within a foreign git repository incorrectly use the foreign git repo's commit hash in the version. This is already a problem (even with #7522 apparently!), so not a blocker for this PR.
  3. Dist dir builds prefix the commit hash with 'g', while git builds do not. This is already a problem, so also not a regression in this PR.
  4. Clean git builds using a different build dir incorrectly tag the version as "-dirty". This should probably be fixed in this PR.
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 6, 2016

Member

BTW seems make translate also needs changes to work out-of-tree. Not necessarily in this pull.

Member

laanwj commented Jun 6, 2016

BTW seems make translate also needs changes to work out-of-tree. Not necessarily in this pull.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 6, 2016

Member

Looks like this is enough:

diff --git a/src/Makefile.qt.include b/src/Makefile.qt.include
index 3b39919..327a3d1 100644
--- a/src/Makefile.qt.include
+++ b/src/Makefile.qt.include
@@ -390,13 +390,13 @@ QT_QM=$(QT_TS:.ts=.qm)

 SECONDARY: $(QT_QM)

-qt/bitcoinstrings.cpp: $(libbitcoin_server_a_SOURCES) $(libbitcoin_wallet_a_SOURCES)
+$(srcdir)/qt/bitcoinstrings.cpp: $(libbitcoin_server_a_SOURCES) $(libbitcoin_wallet_a_SOURCES)
        @test -n $(XGETTEXT) || echo "xgettext is required for updating translations"
        $(AM_V_GEN) cd $(srcdir); XGETTEXT=$(XGETTEXT) PACKAGE_NAME="$(PACKAGE_NAME)" COPYRIGHT_HOLDERS="$(COPYRIGHT_HOLDERS)" COPYRIGHT_HOLDERS_SUBSTITUTION="$(COPYRIGHT_HOLDERS_SUBSTITUTION)" $(PYTHON) ../share/qt/extract_strings_qt.py $^

-translate: qt/bitcoinstrings.cpp $(QT_FORMS_UI) $(QT_FORMS_UI) $(BITCOIN_QT_CPP) $(BITCOIN_QT_H) $(BITCOIN_MM)
+translate: $(srcdir)/qt/bitcoinstrings.cpp $(QT_FORMS_UI) $(QT_FORMS_UI) $(BITCOIN_QT_CPP) $(BITCOIN_QT_H) $(BITCOIN_MM)
        @test -n $(LUPDATE) || echo "lupdate is required for updating translations"
-       $(AM_V_GEN) QT_SELECT=$(QT_SELECT) $(LUPDATE) $^ -locations relative -no-obsolete -ts qt/locale/bitcoin_en.ts
+       $(AM_V_GEN) QT_SELECT=$(QT_SELECT) $(LUPDATE) $^ -locations relative -no-obsolete -ts $(srcdir)/qt/locale/bitcoin_en.ts

 $(QT_QRC_LOCALE_CPP): $(QT_QRC_LOCALE) $(QT_QM)
        @test -f $(RCC)

Updated: bitcoin_en.ts was written to the build directory instead of the source directory, now fixed.

Also available at: https://github.com/laanwj/bitcoin/tree/2016_06_translate_out_of_tree

Member

laanwj commented Jun 6, 2016

Looks like this is enough:

diff --git a/src/Makefile.qt.include b/src/Makefile.qt.include
index 3b39919..327a3d1 100644
--- a/src/Makefile.qt.include
+++ b/src/Makefile.qt.include
@@ -390,13 +390,13 @@ QT_QM=$(QT_TS:.ts=.qm)

 SECONDARY: $(QT_QM)

-qt/bitcoinstrings.cpp: $(libbitcoin_server_a_SOURCES) $(libbitcoin_wallet_a_SOURCES)
+$(srcdir)/qt/bitcoinstrings.cpp: $(libbitcoin_server_a_SOURCES) $(libbitcoin_wallet_a_SOURCES)
        @test -n $(XGETTEXT) || echo "xgettext is required for updating translations"
        $(AM_V_GEN) cd $(srcdir); XGETTEXT=$(XGETTEXT) PACKAGE_NAME="$(PACKAGE_NAME)" COPYRIGHT_HOLDERS="$(COPYRIGHT_HOLDERS)" COPYRIGHT_HOLDERS_SUBSTITUTION="$(COPYRIGHT_HOLDERS_SUBSTITUTION)" $(PYTHON) ../share/qt/extract_strings_qt.py $^

-translate: qt/bitcoinstrings.cpp $(QT_FORMS_UI) $(QT_FORMS_UI) $(BITCOIN_QT_CPP) $(BITCOIN_QT_H) $(BITCOIN_MM)
+translate: $(srcdir)/qt/bitcoinstrings.cpp $(QT_FORMS_UI) $(QT_FORMS_UI) $(BITCOIN_QT_CPP) $(BITCOIN_QT_H) $(BITCOIN_MM)
        @test -n $(LUPDATE) || echo "lupdate is required for updating translations"
-       $(AM_V_GEN) QT_SELECT=$(QT_SELECT) $(LUPDATE) $^ -locations relative -no-obsolete -ts qt/locale/bitcoin_en.ts
+       $(AM_V_GEN) QT_SELECT=$(QT_SELECT) $(LUPDATE) $^ -locations relative -no-obsolete -ts $(srcdir)/qt/locale/bitcoin_en.ts

 $(QT_QRC_LOCALE_CPP): $(QT_QRC_LOCALE) $(QT_QM)
        @test -f $(RCC)

Updated: bitcoin_en.ts was written to the build directory instead of the source directory, now fixed.

Also available at: https://github.com/laanwj/bitcoin/tree/2016_06_translate_out_of_tree

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Jun 6, 2016

Member

@luke-jr Whoa, that's great feedback! Thanks for the testing. I'm having a look at the "-dirty" issue.

@laanwj Thanks, I'll add that here.

Member

theuni commented Jun 6, 2016

@luke-jr Whoa, that's great feedback! Thanks for the testing. I'm having a look at the "-dirty" issue.

@laanwj Thanks, I'll add that here.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 9, 2016

Member

Let's try to get this merged soon.
I can PR the make translate change separately if that's mroe convenient.

Member

laanwj commented Jun 9, 2016

Let's try to get this merged soon.
I can PR the make translate change separately if that's mroe convenient.

@laanwj laanwj added this to the 0.13.0 milestone Jun 9, 2016

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Jun 9, 2016

Member

@luke-jr I believe I've found the issue here. When you "chown -R root:root .", you're setting the git index to read-only. That makes "git diff" fail to update the index, which makes the diff-index fail.

For the sake of this PR, I'm ok with that not working correctly, since git is in a pretty bad state.

Member

theuni commented Jun 9, 2016

@luke-jr I believe I've found the issue here. When you "chown -R root:root .", you're setting the git index to read-only. That makes "git diff" fail to update the index, which makes the diff-index fail.

For the sake of this PR, I'm ok with that not working correctly, since git is in a pretty bad state.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 10, 2016

Member

since git is in a pretty bad state

That sounds ominous. Time to switch to mercurial or bzr? :-)

Member

laanwj commented Jun 10, 2016

since git is in a pretty bad state

That sounds ominous. Time to switch to mercurial or bzr? :-)

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 10, 2016

Member

"works for me" ACK d1a3d57

Member

laanwj commented Jun 10, 2016

"works for me" ACK d1a3d57

@laanwj laanwj merged commit d1a3d57 into bitcoin:master Jun 10, 2016

1 check passed

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

laanwj added a commit that referenced this pull request Jun 10, 2016

Merge #8133: build: Finish up out-of-tree changes
d1a3d57 bulid: fix "make translate" when out-of-tree (Cory Fields)
340012d build: add temporary fix for "bad magic number" error in out-of-tree builds (Cory Fields)
142ffc7 travis: use out-of-tree build (Cory Fields)
92e37a3 build: fix out-of-tree 'make deploy' for osx (Cory Fields)
ab95d5d build: a few ugly hacks to get the rpc tests working out-of-tree (Cory Fields)
fc4ad0c build: more out-of-tree fixups (Cory Fields)
0cb0f26 build: out-of-tree fixups (Cory Fields)
@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Jun 10, 2016

Member

Haha. That was a half-complete thought as usual.

Should read: "I'm ok with that not working correctly, since if you're hitting this case, your git repo is already unreliable".

Member

theuni commented Jun 10, 2016

Haha. That was a half-complete thought as usual.

Should read: "I'm ok with that not working correctly, since if you're hitting this case, your git repo is already unreliable".

@theuni theuni deleted the theuni:out-of-tree-clean branch Jun 10, 2016

@laanwj laanwj referenced this pull request Jun 14, 2016

Closed

Remaining minor issues for out-of-tree build #7937

0 of 3 tasks complete

@jnewbery jnewbery referenced this pull request Jan 31, 2017

Merged

Improve rpc-tests.py #9657

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge #8133: build: Finish up out-of-tree changes
d1a3d57 bulid: fix "make translate" when out-of-tree (Cory Fields)
340012d build: add temporary fix for "bad magic number" error in out-of-tree builds (Cory Fields)
142ffc7 travis: use out-of-tree build (Cory Fields)
92e37a3 build: fix out-of-tree 'make deploy' for osx (Cory Fields)
ab95d5d build: a few ugly hacks to get the rpc tests working out-of-tree (Cory Fields)
fc4ad0c build: more out-of-tree fixups (Cory Fields)
0cb0f26 build: out-of-tree fixups (Cory Fields)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge #8133: build: Finish up out-of-tree changes
d1a3d57 bulid: fix "make translate" when out-of-tree (Cory Fields)
340012d build: add temporary fix for "bad magic number" error in out-of-tree builds (Cory Fields)
142ffc7 travis: use out-of-tree build (Cory Fields)
92e37a3 build: fix out-of-tree 'make deploy' for osx (Cory Fields)
ab95d5d build: a few ugly hacks to get the rpc tests working out-of-tree (Cory Fields)
fc4ad0c build: more out-of-tree fixups (Cory Fields)
0cb0f26 build: out-of-tree fixups (Cory Fields)

codablock added a commit to codablock/dash that referenced this pull request Dec 22, 2017

Merge #8133: build: Finish up out-of-tree changes
d1a3d57 bulid: fix "make translate" when out-of-tree (Cory Fields)
340012d build: add temporary fix for "bad magic number" error in out-of-tree builds (Cory Fields)
142ffc7 travis: use out-of-tree build (Cory Fields)
92e37a3 build: fix out-of-tree 'make deploy' for osx (Cory Fields)
ab95d5d build: a few ugly hacks to get the rpc tests working out-of-tree (Cory Fields)
fc4ad0c build: more out-of-tree fixups (Cory Fields)
0cb0f26 build: out-of-tree fixups (Cory Fields)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment