-
Notifications
You must be signed in to change notification settings - Fork 19
build: integrate duktape #235
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
endif() | ||
FetchContent_Declare( | ||
duktape | ||
URL https://github.com/svaarala/duktape/releases/download/v2.7.0/duktape-2.7.0.tar.xz |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait since when is it our policy to download packages for the user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. There are two things involved here. The reason I chose this integration option and the policies for integration.
The problem with usual vcpkg + find_package integration:
The first thing I tried was vcpkg, but duktape + vcpkg has a few problems. 1) duktape doesn't provide any CMake integration at all, which makes the vcpkg package much less helpful, but the biggest problem is 2) the vcpkg recipe requires a few host dependencies, including some specific python2 in case the user chooses some custom configuration. But this custom configuration is not something we need so we would be requiring python2 for nothing, which is even hard to setup in some environments. These host dependencies kind of make vcpkg useless, because it's basically vcpkg throwing the towel on that dependency.
The implementation:
So this integration includes two options. 1) The user can provide the duktape source directory, which is similar to how b2 handles zlib, and 2) it falls back to fetch content to download the source if the user doesn't provide it. This is what seemed most logical.
We could remove the second option, but I don't see how that improves things. It doesn't add any functionality, makes the build process in CI or elsewhere more complex, and fetch content is a very usual and totally ok alternative to fallback to. Fetch content is only controversial as the first option because we obviously want to use the version the user already has.
The policy:
In terms of "policy", the only reason why Boost avoids the usual CMake integration of other libraries at all costs while the rest of the world does it is because of b2
, where neither vcpkg integration nor fetch content can be replicated properly, and because of the super project build, which unfortunately requires libraries to have no non-optional dependencies.
By "usual" CMake integration I mean find_package falling back to FetchContent. This falling back even happens automatically in the latest versions of CMake. And I say "unfortunately" when talking about no dependencies because optional dependencies as a default are bad practice in other projects in terms of testing since it creates an exponential explosion that's impossible to test.
In any case, none of the conditions from boost (b2 equivalence and a superproject with no mandatory dependencies) apply to this project or basically any project outside boost.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have another problem which is _ITERATOR_DEBUG_LEVEL
and the building and linking with optimized versus non-optimized builds for Debug, DebugWIthRel, and RelWithDebInfo.
We need another commit that completely removes the "--adoc" command line option |
Oh... OK. I thought you just wanted me to remove the |
This PR includes a subset of #228 as a starting to point to allow us to make better design decisions along the way.
What this PR includes from #228
What this PR DOES NOT includes from #228