Automake file bug fix - src/leveldb/ and src/leveldb.cpp problem

brandondahler commented at 3:07 am on November 6, 2013: contributor

Fixes issue #3200.

I do not know much about autotools nor automake, I would highly recommend review by a more knowledgeable person in the area.

Has been merged into #2767 and subsequenty caused it to pass the BitcoinPullTester.

in src/Makefile.am: in 1222b0cc31 outdated

80@@ -81,7 +81,7 @@ CLEANFILES = leveldb/libleveldb.a leveldb/libmemenv.a *.gcda *.gcno
81 
82 DISTCLEANFILES = obj/build.h
83 
84-EXTRA_DIST = leveldb Makefile.include
85+EXTRA_DIST = leveldb/* Makefile.include

laanwj commented at 3:14 pm on November 6, 2013:

This is about the files that are included in a distribution that is created with “make dist”?

I think * is too general, it’s better to be somewhat more specific about the type of files to include (ie, *.cc, *.h) otherwise built objects and libraries and other temporary stuff get included in the distribution too.

brandondahler commented at 6:15 pm on November 6, 2013:

To my understanding the previous version did the same thing–copied the leveldb folder recursively.

On Nov 6, 2013, at 9:14 AM, “Wladimir J. van der Laan” notifications@github.com wrote:

In src/Makefile.am:

@@ -81,7 +81,7 @@ CLEANFILES = leveldb/libleveldb.a leveldb/libmemenv.a *.gcda *.gcno

DISTCLEANFILES = obj/build.h

-EXTRA_DIST = leveldb Makefile.include +EXTRA_DIST = leveldb/* Makefile.include This is about the files that are included in a distribution that is created with “make dist”?

I think * is too general, it’s better to be somewhat more specific about the type of files to include (ie, *.cc, *.h) otherwise built objects and libraries and other temporary stuff get included in the distribution too.

— Reply to this email directly or view it on GitHub.

laanwj commented at 6:42 pm on November 6, 2013:

Hmm If this is a recursive copy (and not just one level deep), I don’t understand why the /* is needed.

brandondahler commented at 0:10 am on November 7, 2013:

Please see the linked issue from the original post.

On Nov 6, 2013, at 12:42 PM, “Wladimir J. van der Laan” notifications@github.com wrote:

In src/Makefile.am:

@@ -81,7 +81,7 @@ CLEANFILES = leveldb/libleveldb.a leveldb/libmemenv.a *.gcda *.gcno

DISTCLEANFILES = obj/build.h

-EXTRA_DIST = leveldb Makefile.include +EXTRA_DIST = leveldb/* Makefile.include Hmm If this is a recursive copy (and not just one level deep), I don’t understand why the /* is needed.

— Reply to this email directly or view it on GitHub.

laanwj commented at 8:06 am on November 7, 2013:

Yes, I’ve read that issue, it describes the problem but not why this solution works – the problem is that “leveldb” invoked a rule called “leveldb” and does not include the actual directory recursively? And adding /* works around this? @theuni you’re the automake wizard, do you know what’s going on here?

theuni commented at 9:41 pm on November 7, 2013:

Yea, looks like it’s this:

leveldb.cpp is touched, so it checks to see what needs to be rebuilt. Possible options are leveldb.o, leveldb.exe, leveldb, etc. It finds that leveldb (the folder) is older than leveldb.cpp, so it must be “rebuilt”. The implicit rule tries to build ’leveldb’ (a binary) from leveldb.cpp and obviously fails.

distdir depends on all distributed files:

0distdir: $(DISTFILES)

Changing ’leveldb’ to ’leveldb/*’ (or anything that’s not exactly ’leveldb’) in $EXTRA_DIST removes that dependency from $DISTFILES, so it removes the target as well. That only addresses the symptom though, so I’d rather not go that route.

Possible fixes:

rename leveldb.cpp (my suggestion. This is likely to cause similar head-scratchers in the future otherwise.)
rename leveldb folder. Obviously unrealistic.
add an empty leveldb rule to nullify the implicit one:

0leveldb:

That tells make to do nothing when leveldb needs to be ‘rebuilt’, rather than using the implicit rule of using the .cpp of the same name. It will be ‘rebuilt’ with each build, but it’s empty so it’d be an invisible no-op. Assuming that works as intended, it’d be my second choice.

gavinandresen commented at 0:05 am on November 8, 2013:

NACK on renaming– that will just cause us headaches merging upstream changes.

Empty rule (with a comment– we can add a comment, right?) sounds like the correct fix.

sipa commented at 0:24 am on November 8, 2013:

Renaming leveldb.cpp should be fine though.

jgarzik commented at 0:27 am on November 8, 2013:

ACK on renaming leveldb.cpp

gavinandresen commented at 0:32 am on November 8, 2013:

Ignore that idiot gavin, he thought there was a leveldb/leveldb.cpp ….

brandondahler commented at 2:13 am on November 8, 2013: contributor

Renamed leveldb.{h,cpp} to leveldbwrapper.{h,cpp}, rename class CLevelDB to CLevelDBWrapper.

Reverted original Makefile.am change, updating it with the new file names.

in src/Makefile.am: in 9fd8c5e411 outdated

80@@ -81,7 +81,7 @@ CLEANFILES = leveldb/libleveldb.a leveldb/libmemenv.a *.gcda *.gcno
81 
82 DISTCLEANFILES = obj/build.h
83 
84-EXTRA_DIST = leveldb Makefile.include
85+EXTRA_DIST = leveldb Makefile.include

sipa commented at 2:40 am on November 8, 2013:

Trailing space.

sipa commented at 2:40 am on November 8, 2013: member

ACK on changes. Please squash the commits.

in src/leveldbwrapper.h: in 9fd8c5e411 outdated

150@@ -151,4 +151,4 @@ class CLevelDB
151     }
152 };
153 
154-#endif // BITCOIN_LEVELDB_H

theuni commented at 3:07 am on November 8, 2013:

Missed the actual ifdef/define renames.

brandondahler commented at 3:57 am on November 8, 2013: contributor

Rebased, squashed, fixed problems.

Rename leveldb.{h,cpp} to leveldbwrapper.{h,cpp}. b64187d05f

in src/leveldbwrapper.h: in 2cb03dacd2 outdated

0@@ -1,8 +1,8 @@
1 // Copyright (c) 2012 The Bitcoin developers

Diapolo commented at 7:31 am on November 8, 2013:

BitcoinPullTester commented at 0:28 am on November 9, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/b64187d05f327992c304836fe1fc1d276ec80c36 for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

sipa referenced this in commit 7c4c207be8 on Nov 9, 2013

sipa merged this on Nov 9, 2013

sipa closed this on Nov 9, 2013

brandondahler deleted the branch on Nov 9, 2013

Bushstar referenced this in commit 9de994988b on Apr 8, 2020

DrahtBot locked this on Sep 8, 2021

Automake file bug fix - src/leveldb/ and src/leveldb.cpp problem #3211