-
Notifications
You must be signed in to change notification settings - Fork 81
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
[INFRA] Buildsystem #3337
[INFRA] Buildsystem #3337
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3337 +/- ##
=======================================
Coverage 98.11% 98.12%
=======================================
Files 270 270
Lines 11913 11915 +2
Branches 103 103
=======================================
+ Hits 11689 11691 +2
Misses 224 224 ☔ View full report in Codecov by Sentry. |
Documentation preview available at https://docs.seqan.de/preview/seqan/seqan3/3337 |
a8bc18f
to
fd6a80d
Compare
f267454
to
a82b3d5
Compare
cmake/seqan3-config.cmake
Outdated
# cereal::cereal is an interface target and cannot be used in try_compile: | ||
# There is no shared or static library to link against. | ||
set (SEQAN3_TRYCOMPILE_LIBRARIES ${SEQAN3_LIBRARIES}) | ||
list (REMOVE_ITEM SEQAN3_TRYCOMPILE_LIBRARIES cereal::cereal) |
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.
Should we not rather split SEQAN3_LIBRARIES into SEQAN3_INTERFACE_LIBRARIES and SEQAN3_NORMAL_LIBRARIES? And then have SEQAN3_LIBRARIES be the union of these two sets. Thus, we can run the trycompile test only on the normal libraries rather than removing cereal::ceral from SEQAN3_TRYCOMPILE_LIBRARIES. In case we add more interface libraries in the future we make sure not to forget to remove it here.
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.
Depends, you can argue for both :)
SEQAN3_LIBRARIES
isn't a bad name, because you end up using it for target_link_libraries
.
try_compile
for some reason doesn't handle interface libraries; so it makes sense to have SEQAN3_TRYCOMPILE_LIBRARIES
to signal this.
Independent of the approach, you always need to know which libraries are interface only and get the assignment correct.
In any case, try_compile will fail if you try to link an interface lib.
Currently, I find it easier the way it is.
If we are ever to have more than one interface lib, it would certainly make sense to have two variables (interface libs, and non-interface libs).
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.
SEQAN3_LIBRARIES isn't a bad name, because you end up using it for target_link_libraries.
Never said that. That's why I want to keep it.
try_compile for some reason doesn't handle interface libraries; so it makes sense to have SEQAN3_TRYCOMPILE_LIBRARIES to signal this.
sure
Independent of the approach, you always need to know which libraries are interface only and get the assignment correct.
Absolutely. Although one could do this with a function I guess, by checking the type property of the target.
Currently, I find it easier the way it is.
If we are ever to have more than one interface lib, it would certainly make sense to have two variables (interface libs, and non-interface libs).
I see you point. We could also have the removal of interface libraries to generate the SEQAN3_TRYCOMPILE_LIBRARIES automated using a function that checks the type property.
What do you say?
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.
I see you point. We could also have the removal of interface libraries to generate the SEQAN3_TRYCOMPILE_LIBRARIES automated using a function that checks the type property. What do you say?
I'll try that
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, so it has do to something with whether a target is imported or not.
There are four ways.
Either of them works.
I don't like 3). You have to look at cereal to define the library, and with new ones you would need to do the same.
I like 1) and 4) because it's simple.
I like 2) because it's automatic (and hopefully always works). I don't like it because it adds complexity.
I think my order of preference would be
- 4
- 1 == 2
- 3
1) Leave as is
Just strip cereal from the libraries.
2) Generic
# try_compile can only link against imported libraries or paths (/usr/lib/.../librt.a)
# Here, we filter out libraries that cannot be used in try_compile.
# Things that can be found via find_package are usually imported libraries.
# An exception is cereal::cereal:
# * If installed on the system, it is found via find_package, or CPM if CPM_USE_LOCAL_PACKAGES is set.
# In this case, cereal::cereal is an imported target.
# * If cereal is fetched by CPM, it is not an imported target and cannot be used in try_compile.
foreach (LIB IN LISTS SEQAN3_LIBRARIES)
if (TARGET ${LIB})
get_target_property(is_imported ${LIB} IMPORTED)
if (is_imported)
list (APPEND SEQAN3_TRYCOMPILE_LIBRARIES ${LIB})
endif ()
else () # E.g., SEQAN3_RT_LIB
list (APPEND SEQAN3_TRYCOMPILE_LIBRARIES ${LIB})
endif ()
endforeach ()
3) Make cereal imported
CPM just downloads cereal, and then
if (cereal_ADDED)
add_library (cereal INTERFACE IMPORTED)
target_include_directories (cereal SYSTEM INTERFACE "${cereal_SOURCE_DIR}/include")
add_library (cereal::cereal ALIAS cereal)
endif ()
4) Try compile without libs
The libraries are not really needed to compile platform.hpp
.
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.
well, 4 it is then. I think we don't need to do things, that are not necessary.
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.
Otherwise, I'd prefer 2 inside of a seprate function.
Needed when used withour optional libs
326bb1e
to
2cbfd77
Compare
If there is no cereal
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.
So just use try compile without the libs and then this looks good to go! Nice! Thank you!
This makes seqan3 way more maintainable.
The macros for detecting optional dependencies now follow the same naming scheme. They also behave the same way.
Custom option for force enabling/disabling were removed. This is already provided by Cmake!
All dependencies now use
find_package
. In git checkouts, we use CPM for cereal - the user can tell CPM to prefer local packages.We do not vendor any dependencies. Things we need to vendor (sdsl), are in contrib and do not need special handling.
Not vendoring makes packaging way easier. If you install seqan3 and want to use cereal with the installed seqan3, cereal also needs to be installed.
In summary this means that we delegate the responsibility for installing dependencies to the package maintainer. Previously, we did hacky stuff to make the package work without installing depenencies, and package maintainers had to patch our package to not do this and to use installed packages instead.