Refactor build; better options for enabling database backends #12049
Conversation
JosiahWI
force-pushed
the
refactor/cmake-database-options
branch
3 times, most recently
from
614abdf
to
ceb4f45
Compare
|
I need help. I don't have a testing environment for this and I can't think of anything else I could've messed up. I've checked documentation for get_filename_component and find_library; I've tried with and without strings around the variables; I've listed the library names explicitly with the right extensions. |
|
There's a right way to do this. There are built-in variables intended for custom library directories, so the fault is with the CI build and I'll be fixing that. EDIT: Crosscompiling complicates things. I decided to do it the convenient way that I never even realized at first, just renaming the variables correctly. |
JosiahWI
force-pushed
the
refactor/cmake-database-options
branch
5 times, most recently
from
6099bb4
to
cf9e760
Compare
Added tristate options with options SHRUG, ON and OFF, and did refactoring.
JosiahWI
force-pushed
the
refactor/cmake-database-options
branch
from
816791f
to
74e3385
Compare
rubenwardy
added
@ Build
| # | ||
| # And the following imported targets: | ||
| # | ||
| # leveldb::leveldb |
There was a problem hiding this comment.
What about making the target & file name follow the official naming (LevelDB)?
as for the options you have free choice right? because keeping them uppercase would keep old build scripts working as before.
There was a problem hiding this comment.
The target naming was intentional to match the targets these projects export in their master branches, so that in the long term we can switch to using those projects' configs without having to update the target names all over the place. Good point about the build scripts. The options should stay the same as before, like you said.
|
|
||
|
|
||
| set(ENABLE_POSTGRESQL SHRUG CACHE STRING "Enable PostgreSQL backend") | ||
| set_property(CACHE ENABLE_POSTGRESQL PROPERTY STRINGS SHRUG ON OFF) |
There was a problem hiding this comment.
my suggestion to keep compatibility with how these options worked before:
- REQUIRED = search and error if missing
- ON / TRUE /
1= search but dont error - OFF / FALSE /
0= dont search
There was a problem hiding this comment.
One of the objectives of the PR is to change the way this works. Description has been updated.
There was a problem hiding this comment.
I see there's a potentially big problem here because it could break compatibility with people's build scripts. It would probably be almost entirely alleviated just by allowing REQUIRED and making it an alias for ON, but perhaps it's still a bit unprecedented. What are your thoughts on this? Is it worth making the build system simpler if the options no longer work the same way? Maybe it would be possible to allow setting an option to switch to the new behavior, and then deprecate the old behavior after 1 or 2 more major releases or something.
| -DLEVELDB_DLL=$libdir/leveldb/bin/libleveldb.dll | ||
| -Dleveldb_INCLUDE_DIR=$libdir/leveldb/include \ | ||
| -Dleveldb_LIBRARY=$libdir/leveldb/lib/libleveldb.dll.a \ | ||
| -Dleveldb_DLL=$libdir/leveldb/lib/libleveldb.dll |
sfan5
added
the
Action / change needed
Fixes #12026 and fixes #12045
Edit: The point of the PR is to change the behavior from "ON - use if found; OFF - do not use; REQUIRED - error if not found" to "ON - error if not found; OFF - do not use; AUTO (currently SHRUG, will be changed) - enable if found"
The behavior of ON right now is not intuitive. Everyone assumes that "ON" means "ON", not "maybe ON". This leads to people not knowing whether the option worked, because they don't know they have to scroll back through the CMake log to see. If someone turns a feature on, they are generally going to assume it is required. If we want to have this kind of "use if found" behavior, it should be internal to the build, or at least named something that indicates what it does, and it should be possible for the user to disable it.
How to test
Build with the different options and observe that it works as expected.