Skip to content
This repository has been archived by the owner on Jun 17, 2022. It is now read-only.

Split sources for easier buildsystem integration #19

Merged
merged 2 commits into from
Oct 4, 2021

Conversation

theuni
Copy link

@theuni theuni commented Jun 17, 2019

This addresses issues like the one in bitcoin/bitcoin#12467, where some of our compiler flags end up being dropped during the subconfigure of Univalue. Specifically, we're still using the compiler-default c++ version rather than forcing c++17.

We can drop the need subconfigure completely in favor of a tighter build integration, where the sources are listed separately from the build recipes, so that they may be included directly by upstream projects. This is similar to the way leveldb build integration works in Core.

A similar split was recently added for minisketch: sipa/minisketch#8

Upstream benefits of this approach include:

  • Better caching (for ex. ccache and autoconf)
  • No need for a slow subconfigure
  • No more missing compile flags
  • Compile only the objects needed

There are no benefits to Univalue itself that I can think of. These changes should be a no-op here, and to downstreams as well until they take advantage of the new sources.mk. Once merged, This Core branch is ready-ish for PR, and takes advantage of the features added here.

libsecp256k1 will get the same treatment once this is done.

@fanquake
Copy link
Member

Concept ACK

This allows for tighter integration with upstream projects. Otherwise it should
be a no-op.
@theuni
Copy link
Author

theuni commented Jul 15, 2021

Rebased

theuni added a commit to theuni/bitcoin that referenced this pull request Jul 15, 2021
Requires bitcoin-core/univalue-subtree#19 .

This addresses issues like the one in bitcoin#12467, where some of our compiler flags
end up being dropped during the subconfigure of Univalue. Specifically, we're
still using the compiler-default c++ version rather than forcing c++11.

We can drop the need subconfigure completely in favor of a tighter build
integration, where the sources are listed separately from the build recipes,
so that they may be included directly by upstream projects. This is
similar to the way leveldb build integration works in Core.

Upstream benefits of this approach include:

    Better caching (for ex. ccache and autoconf)
    No need for a slow subconfigure
    No more missing compile flags
    Compile only the objects needed

There are no benefits to Univalue itself that I can think of. These changes
should be a no-op there, and to downstreams as well until they take advantage
of the new sources.mk. Once merged, This Core branch is ready-ish for PR, and
takes advantage of the features added here.

This also removes the option to use an external univalue to avoid similar ABI
issues with mystery binaries.
@theuni
Copy link
Author

theuni commented Jul 15, 2021

Updated the Core implementation here as well: https://github.com/theuni/bitcoin/commits/univalue-split .

theuni added a commit to theuni/bitcoin that referenced this pull request Jul 15, 2021
Requires bitcoin-core/univalue-subtree#19 .

This addresses issues like the one in bitcoin#12467, where some of our compiler flags
end up being dropped during the subconfigure of Univalue. Specifically, we're
still using the compiler-default c++ version rather than forcing c++17.

We can drop the need subconfigure completely in favor of a tighter build
integration, where the sources are listed separately from the build recipes,
so that they may be included directly by upstream projects. This is
similar to the way leveldb build integration works in Core.

Core benefits of this approach include:
- Better caching (for ex. ccache and autoconf)
- No need for a slow subconfigure
- Faster autoconf
- No more missing compile flags
- Compile only the objects needed

There are no benefits to Univalue itself that I can think of. These changes
should be a no-op there, and to downstreams as well until they take advantage
of the new sources.mk.

This also removes the option to use an external univalue to avoid similar ABI
issues with mystery binaries.
@hebasto
Copy link
Member

hebasto commented Jul 17, 2021

Concept ACK.

What is the reason to keep the lib/univalue_escapes.h auto-generated header in the git repo after the first commit?

@theuni
Copy link
Author

theuni commented Jul 26, 2021

@hebasto That's just a convenience Makefile target to regenerate univalue_escapes.h. It's not automatically built in any case. There would be no need for a downstream project to run gen.

That commit is part of this PR because univalue_escapes.h was needlessly used in a recipe, so rather than exposing a variable for it, I eliminated its use entirely.

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Aug 6, 2021
Requires bitcoin-core/univalue-subtree#19 .

This addresses issues like the one in bitcoin#12467, where some of our compiler flags
end up being dropped during the subconfigure of Univalue. Specifically, we're
still using the compiler-default c++ version rather than forcing c++17.

We can drop the need subconfigure completely in favor of a tighter build
integration, where the sources are listed separately from the build recipes,
so that they may be included directly by upstream projects. This is
similar to the way leveldb build integration works in Core.

Core benefits of this approach include:
- Better caching (for ex. ccache and autoconf)
- No need for a slow subconfigure
- Faster autoconf
- No more missing compile flags
- Compile only the objects needed

There are no benefits to Univalue itself that I can think of. These changes
should be a no-op there, and to downstreams as well until they take advantage
of the new sources.mk.

This also removes the option to use an external univalue to avoid similar ABI
issues with mystery binaries.
Copy link

@jnewbery jnewbery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested ACK 4a5b0a1

One small question about FORCE inline.

@@ -30,89 +27,32 @@ $(GENBIN): $(GEN_SRCS)
@echo Building $@
$(AM_V_at)c++ -I$(top_srcdir)/include -o $@ $<

gen: lib/univalue_escapes.h $(GENBIN)
@echo Updating $<
gen: $(GENBIN) FORCE

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this FORCE doing anything? As far as I understand, FORCE has no special meaning and by convention is used for a target that doesn't have any prerequisites or recipe, and is therefore always run: https://www.gnu.org/software/make/manual/html_node/Force-Targets.html#Force-Targets. I think the intention is to make sure that $(GENBIN) is always rebuilt, but I have some questions:

  • doesn't there need to be a FORCE: target defined for this to work? The various examples I've seen have always included an empty FORCE: target.
  • If the intention is to always rebuild $(GENBIN), then shouldn't FORCE be a dependency of $(GENBIN)? If it's a dependency of gen, then gen will get rereun, but it might be using an old version of $(GENBIN).
  • Doesn't .INTERMEDIATE: $(GENBIN) mean that $(GENBIN) will be deleted at the end of the make anyway, and will therefore be rebuilt on each make gen?
  • Doesn't the fact that gen is PHONY mean that it'll be run every make gen?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't there need to be a FORCE: target defined for this to work?

FORCE is a phony variable here used to make gen always run. (But leaving it undeclared would work too, it would still just always run because it would never complete its goal of generating a file called FORCE)

If the intention is to always rebuild $(GENBIN), then shouldn't FORCE be a dependency of $(GENBIN)?

The intention is for $(GENBIN) to be a real target, and for gen to be a phony convenience target that always rebuilds the header.

Doesn't .INTERMEDIATE: $(GENBIN) mean that $(GENBIN) will be deleted at the end of the make anyway, and will therefore be rebuilt on each make gen?

Yup!

Doesn't the fact that gen is PHONY mean that it'll be run every make gen?

Yup!

The idea here is that "gen" is a convenience target for regenerating the univalue_escapes.h file. It is never run automatically, it is just a way to generate a header that's committed to the repo. As such, when a user does make gen, it should always regenerate the file. The current behavior of depending on its own output is broken.

There's no reason to ever keep $(GENBIN) around, so it's an intermediate. make gen essentially never needs to be used, it's possible that its only use is to demonstrate how the .h was generated.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @theuni! All makes sense.

Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 4a5b0a1

@maflcko maflcko merged commit c390ac3 into bitcoin-core:master Oct 4, 2021
fanquake added a commit to fanquake/bitcoin that referenced this pull request Oct 4, 2021
c390ac375f Merge bitcoin-core/univalue-subtree#19: Split sources for easier buildsystem integration
4a5b0a1c65 build: Move source entries out to sources.mk
6c7d94b33c build: cleanup wonky gen usage
a222637c6d Merge bitcoin#23: Merge changes from jgarzik/univalue@1ae6a23
f77d0f718d Merge commit '1ae6a231a0169938eb3972c1d48dd17cba5947e1' into HEAD
1ae6a231a0 Merge pull request bitcoin#57 from MarcoFalke/test_fix
92bdd11f0b univalue_write: remove unneeded sstream.h include
ffb621c130 Merge pull request bitcoin#56 from drodil/remove_sstream_header
f33acf9fe8 Merge commit '7890db9~' into HEAD
66e0adec4d Remove unnecessary sstream header from univalue.h
88967f6586 Version 1.0.4
1dc113dbef Merge pull request bitcoin#50 from luke-jr/pushKV_bool
72392fb227 [tests] test pushKV for boolean values
c23132bcf4 Pushing boolean value to univalue correctly
81faab26a1 Merge pull request bitcoin#48 from fwolfst/47-UPDATE_MIT_LINK_TO_HTTPS
b17634ef24 Update URLs to MIT license.
88ab64f6b5 Merge pull request bitcoin#46 from jasonbcox/master
35ed96da31 Merge pull request bitcoin#44 from MarcoFalke/Mf1709-univalue-cherrypick-explicit
420c226290 Merge pull request bitcoin#45 from MarcoFalke/Mf1710-univalue-revert-test

git-subtree-dir: src/univalue
git-subtree-split: c390ac375f2fad0433c8fb1b4b5e30749e112a89
fanquake added a commit to fanquake/bitcoin that referenced this pull request Oct 11, 2021
a44caf65fe Merge bitcoin-core/univalue-subtree#28: Import fixes for sanitizer reported issues
135254331e Import fixes for sanitizer reported issues
d5fb86940e refactor: use c++11 range based for loop in checkObject
ff9c379304 refactor: Use nullptr (c++11) instead of NULL
08a99754d5 build: use ax_cxx_compile_stdcxx.m4 to check for C++11 support
66d3713ce7 Merge bitcoin-core/univalue-subtree#29: ci: travis -> cirrus
808d487292 ci: travis -> cirrus
c390ac375f Merge bitcoin-core/univalue-subtree#19: Split sources for easier buildsystem integration
4a5b0a1c65 build: Move source entries out to sources.mk
6c7d94b33c build: cleanup wonky gen usage
a222637c6d Merge bitcoin#23: Merge changes from jgarzik/univalue@1ae6a23
f77d0f718d Merge commit '1ae6a231a0169938eb3972c1d48dd17cba5947e1' into HEAD
1ae6a231a0 Merge pull request bitcoin#57 from MarcoFalke/test_fix
92bdd11f0b univalue_write: remove unneeded sstream.h include
ffb621c130 Merge pull request bitcoin#56 from drodil/remove_sstream_header
f33acf9fe8 Merge commit '7890db9~' into HEAD
66e0adec4d Remove unnecessary sstream header from univalue.h
88967f6586 Version 1.0.4
1dc113dbef Merge pull request bitcoin#50 from luke-jr/pushKV_bool
72392fb227 [tests] test pushKV for boolean values
c23132bcf4 Pushing boolean value to univalue correctly
81faab26a1 Merge pull request bitcoin#48 from fwolfst/47-UPDATE_MIT_LINK_TO_HTTPS
b17634ef24 Update URLs to MIT license.
88ab64f6b5 Merge pull request bitcoin#46 from jasonbcox/master
35ed96da31 Merge pull request bitcoin#44 from MarcoFalke/Mf1709-univalue-cherrypick-explicit
420c226290 Merge pull request bitcoin#45 from MarcoFalke/Mf1710-univalue-revert-test

git-subtree-dir: src/univalue
git-subtree-split: a44caf65fe55b9dd8ddb08f04c0f70409efd53b3
patricklodder added a commit to patricklodder/dogecoin that referenced this pull request Oct 17, 2021
a44caf65f Merge bitcoin-core/univalue-subtree#28: Import fixes for sanitizer reported issues
135254331 Import fixes for sanitizer reported issues
d5fb86940 refactor: use c++11 range based for loop in checkObject
ff9c37930 refactor: Use nullptr (c++11) instead of NULL
08a99754d build: use ax_cxx_compile_stdcxx.m4 to check for C++11 support
66d3713ce Merge bitcoin-core/univalue-subtree#29: ci: travis -> cirrus
808d48729 ci: travis -> cirrus
c390ac375 Merge bitcoin-core/univalue-subtree#19: Split sources for easier buildsystem integration
4a5b0a1c6 build: Move source entries out to sources.mk
6c7d94b33 build: cleanup wonky gen usage
a222637 Merge dogecoin#23: Merge changes from jgarzik/univalue@1ae6a23
98fadc0 Merge dogecoin#24: Push bool into array correctly
5f03f1f Push bool into array correctly
f77d0f7 Merge commit '1ae6a231a0169938eb3972c1d48dd17cba5947e1' into HEAD
98261b1 Merge dogecoin#22: Clamp JSON object depth to PHP limit
54c4015 Clamp JSON object depth to PHP limit
5a58a46 Merge dogecoin#21: Remove hand-coded UniValue destructor.
b4cdfc4 Remove hand-coded UniValue destructor.
1ae6a23 Merge pull request dogecoin#57 from MarcoFalke/test_fix
92bdd11 univalue_write: remove unneeded sstream.h include
ffb621c Merge pull request dogecoin#56 from drodil/remove_sstream_header
f33acf9 Merge commit '7890db9~' into HEAD
7fba60b Merge dogecoin#17: [docs] Update readme
4577454 Merge dogecoin#13: Fix typo
66e0ade Remove unnecessary sstream header from univalue.h
ac7e73c [docs] Update readme
7890db9 Merge #11: Remove deprecated std pair wrappers
88967f6 Version 1.0.4
40e3485 Merge dogecoin#14: Cleaned up namespace imports to reduce symbol collisions
1dc113d Merge pull request dogecoin#50 from luke-jr/pushKV_bool
72392fb [tests] test pushKV for boolean values
c23132b Pushing boolean value to univalue correctly
4a49647 Fix typo
85052a4 Remove deprecated std::pair wrappers
81faab2 Merge pull request dogecoin#48 from fwolfst/47-UPDATE_MIT_LINK_TO_HTTPS
b17634e Update URLs to MIT license.
51d3ab3 Merge #10: Add pushKV(key, boolean) function (replaces #5)
129bad9 [tests] test pushKV for boolean values
b3c44c9 Pushing boolean value to univalue correctly
07947ff Merge #9: [tests] Fix BOOST_CHECK_THROW macro
ec849d9 [tests] Fix BOOST_CHECK_THROW macro
88ab64f Merge pull request dogecoin#46 from jasonbcox/master
35ed96d Merge pull request dogecoin#44 from MarcoFalke/Mf1709-univalue-cherrypick-explicit
420c226 Merge pull request dogecoin#45 from MarcoFalke/Mf1710-univalue-revert-test
d208f98 Cleaned up namespace imports to reduce symbol collisions
31bc9f5 Merge #8: Remove unused Homebrew workaround
fa04209 Remove HomeBrew workaround
a523e08 Merge #7: Declare single-argument (non-converting) constructors "explicit"
a9e53b3 Merge #4: Pull upstream
fe805ea Declare single-argument (non-converting) constructors "explicit"
8a2d6f1 Merge pull request dogecoin#41 from jgarzik/get-obj-map
ba341a2 Add getObjMap() helper method.  Also, constify checkObject().
ceb1194 Handle .pushKV() and .checkObject() edge cases.
107db98 Add ::push_back(double) method for feature parity.
d415300 Move one-line implementation of UniValue::read() to header.
52e85b3 Move exception-throwing get_* methods into separate implementation module.
dac5296 README.md: update code quotes
3e31dcf README.md: close code quote
d09b842 Update README.md
f1b86ed Convert README to markdown style.
1dfe464 Import UniValue class unit tests from bitcoin project.
0d3e74d operator[] takes size_t index parameter (versus unsigned int)
640158f Private findKey() method becomes size_t clean, and returns bool on failure.
7099135 Merge pull request dogecoin#36 from ryanofsky/pr/end-str
a31231b Version 1.0.3
4fd5444 Reject unterminated strings
81eba33 Merge pull request dogecoin#26 from isle2983/pushBackHelpers
3640541 Merge PR dogecoin#32 from branch 'nul-not-special' of git://github.com/ryanofsky/univalue into merge
89bb073 Merge pull request dogecoin#31 from ryanofsky/raw-literals
511008c Merge pull request dogecoin#30 from ryanofsky/test-driver
77974f3 Merge pull request dogecoin#34 from paveljanik/20161116_Wshadow_codepoint
a38fcd3 Do not shadow member variable codepoint.
fd32d1a Don't require nul-terminated string inputs
0bb1439 Support parsing raw literals in UniValue
28876d0 Merge pull request dogecoin#29 from btcdrak/exportspace
839ccd7 Add test driver for JSONTestSuite
26ef3ff Remove trailing whitespace from JSON export
cfa0384 Convenience wrappers for push_back-ing integer types

git-subtree-dir: src/univalue
git-subtree-split: a44caf65fe55b9dd8ddb08f04c0f70409efd53b3
janus pushed a commit to BitgesellOfficial/bitgesell that referenced this pull request Nov 11, 2021
a44caf65fe Merge bitcoin-core/univalue-subtree#28: Import fixes for sanitizer reported issues
135254331e Import fixes for sanitizer reported issues
d5fb86940e refactor: use c++11 range based for loop in checkObject
ff9c379304 refactor: Use nullptr (c++11) instead of NULL
08a99754d5 build: use ax_cxx_compile_stdcxx.m4 to check for C++11 support
66d3713ce7 Merge bitcoin-core/univalue-subtree#29: ci: travis -> cirrus
808d487292 ci: travis -> cirrus
c390ac375f Merge bitcoin-core/univalue-subtree#19: Split sources for easier buildsystem integration
4a5b0a1c65 build: Move source entries out to sources.mk
6c7d94b33c build: cleanup wonky gen usage
a222637c6d Merge #23: Merge changes from jgarzik/univalue@1ae6a23
f77d0f718d Merge commit '1ae6a231a0169938eb3972c1d48dd17cba5947e1' into HEAD
1ae6a231a0 Merge pull request #57 from MarcoFalke/test_fix
92bdd11f0b univalue_write: remove unneeded sstream.h include
ffb621c130 Merge pull request #56 from drodil/remove_sstream_header
f33acf9fe8 Merge commit '7890db9~' into HEAD
66e0adec4d Remove unnecessary sstream header from univalue.h
88967f6586 Version 1.0.4
1dc113dbef Merge pull request #50 from luke-jr/pushKV_bool
72392fb227 [tests] test pushKV for boolean values
c23132bcf4 Pushing boolean value to univalue correctly
81faab26a1 Merge pull request #48 from fwolfst/47-UPDATE_MIT_LINK_TO_HTTPS
b17634ef24 Update URLs to MIT license.
88ab64f6b5 Merge pull request #46 from jasonbcox/master
35ed96da31 Merge pull request #44 from MarcoFalke/Mf1709-univalue-cherrypick-explicit
420c226290 Merge pull request #45 from MarcoFalke/Mf1710-univalue-revert-test

git-subtree-dir: src/univalue
git-subtree-split: a44caf65fe55b9dd8ddb08f04c0f70409efd53b3
vmta added a commit to umkoin/umkoin that referenced this pull request May 15, 2022
68c8f5532 Merge bitcoin-core/univalue-subtree#34: doc: remove TODO
297c53a5e doc: remove TODO
6c19d050a Merge bitcoin-core/univalue-subtree#33: Add getInt<Integral>() helper
09e4a930f Add getInt helper
10619e0d9 Merge bitcoin-core/univalue-subtree#32: refactor: include-what-you-use
431cdf5d2 refactor: use constexpr where appropriate
64fc881fa refactor: cleanup headers for iwyu
9c35bf38e Merge bitcoin-core/univalue-subtree#30: doc: note that our API has diverged from upstream
09b65facb doc: note that our API has diverged from upstream
a44caf65f Merge bitcoin-core/univalue-subtree#28: Import fixes for sanitizer reported issues
135254331 Import fixes for sanitizer reported issues
d5fb86940 refactor: use c++11 range based for loop in checkObject
ff9c37930 refactor: Use nullptr (c++11) instead of NULL
08a99754d build: use ax_cxx_compile_stdcxx.m4 to check for C++11 support
66d3713ce Merge bitcoin-core/univalue-subtree#29: ci: travis -> cirrus
808d48729 ci: travis -> cirrus
c390ac375 Merge bitcoin-core/univalue-subtree#19: Split sources for easier buildsystem integration
4a5b0a1c6 build: Move source entries out to sources.mk
6c7d94b33 build: cleanup wonky gen usage

git-subtree-dir: src/univalue
git-subtree-split: 68c8f5532d02203ec37eb4b2baf8492249f73ab6
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants