Skip to content
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

The end of header only #16

Merged
merged 6 commits into from
Oct 28, 2021
Merged

The end of header only #16

merged 6 commits into from
Oct 28, 2021

Conversation

uphoffc
Copy link
Contributor

@uphoffc uphoffc commented Sep 3, 2021

The goal of this pull request is to make easi a non-header-only - i.e. classic - library. Why? Because header-only is essentially BS for easi and I dunno why I did that in the first place.

Pro:

  • faster compile times in user code
  • symbols are only defined once (You were in deep trouble if you include easi in two separate libs and then try to link these together. The problem is real in exahype which is why there is a branch compiled_lib, which fixes some problems but is only halfway finished.)
  • header-only was a garbage idea from the beginning as you have to link against yaml-cpp and impalajit such that you cannot just drop in the easi headers and be done - which is the whole point of header-only

This PR is pretty heavy but there are no actual code changes besides moving things around. (With the exception of YAMLComponentParsers.) However, almost every line of code changes, because

  • Well I had to move code in the cpps
  • I fixed all includes while I was at it
  • I applied clang-format for extra beauty (yes it messes up the history a bit but I think the history is not so valuable in this project)

Moreover, I started to create some nice install stuff. The ultimate goal would be that you can write

find_package (easi)
add_executable (test test.cpp)
target_link_libraries(test easi::easi)

in your user project. However, that does not quite work yet due to the dependencies. I'll further look into this but I would be nice to get some help from @krenzland here (after his vacation!!1).

@uphoffc uphoffc marked this pull request as draft September 3, 2021 17:14
@Thomas-Ulrich
Copy link
Contributor

Hi,
That's sounds great.
But why not applying clang formatting to the current master branch, and then merge your changes by the updated master (at least give a try)?
That would facilitate reviewing your PR (not that I have the capacities to do that ^^).

@uphoffc
Copy link
Contributor Author

uphoffc commented Sep 8, 2021

Ah I guess that's tricky to do, but the following should do what you request:
https://github.com/SeisSol/easi/compare/606357c0a193c1d10a5d513ea3ef872ec7277b22..66d3cd7f01f01a5c17f5df7072b9ae6efef588c9

@Thomas-Ulrich
Copy link
Contributor

Hi,
Do you need some testing on this PR? How can I help?

@uphoffc
Copy link
Contributor Author

uphoffc commented Oct 26, 2021

Unit tests still run :-)

@Thomas-Ulrich
Copy link
Contributor

Then should we merge?

@uphoffc
Copy link
Contributor Author

uphoffc commented Oct 26, 2021

@krenzland is going to have a look at the cmake part then we merge

@Thomas-Ulrich
Copy link
Contributor

How difficult would it be to have python bindings when the PR is merged? Would be quite useful ;-)

@uphoffc
Copy link
Contributor Author

uphoffc commented Oct 26, 2021

Wouldn't do it with this PR but its a good idea!

@krenzland
Copy link
Contributor

Python bindings are relatively easy and can even be auto-generated with e.g. https://github.com/pybind/pybind11

Copy link
Contributor

@krenzland krenzland left a comment

Choose a reason for hiding this comment

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

Some very small things. Not sure if everything is perfect but it is definitely better than the status quo :)
Thanks!

CMakeLists.txt Outdated
project (easi)
cmake_minimum_required (VERSION 3.8)
cmake_minimum_required (VERSION 3.9)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we require a more modern version? 3.9 is pretty old...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

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

3.16 is on NG. The workstations at LRZ have 3.10 at least. I have 3.20 on my laptop.
-> Use the version that you are using to test it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tested 3.13

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@uphoffc uphoffc marked this pull request as ready for review October 28, 2021 07:53
@krenzland krenzland merged commit ce945bf into master Oct 28, 2021
@krenzland krenzland deleted the carsten/the-end-of-header-only branch October 28, 2021 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants