-
Notifications
You must be signed in to change notification settings - Fork 26
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
Modified cmake projects #155
Conversation
Codecov Report
@@ Coverage Diff @@
## master #155 +/- ##
=======================================
Coverage 99.25% 99.25%
=======================================
Files 6 6
Lines 4809 4809
=======================================
Hits 4773 4773
Misses 36 36 |
934bdb1
to
8fcf55e
Compare
8fcf55e
to
9dc6478
Compare
c4ed9e0
to
ae61f44
Compare
Co-authored-by: Philip Top <top1@llnl.gov> Co-authored-by: HELICS-bot <HELICS-bot@users.noreply.github.com>
b7e48bf
to
6ffdb26
Compare
6ffdb26
to
2bc410b
Compare
413661f
to
c27430c
Compare
c27430c
to
636880f
Compare
finish up fixes and recommendations from #153 and add tests for the shared library and install packages |
@kerim371 can you take a look through these changes and see if they match up with your recommendations |
Co-authored-by: Philip Top <top1@llnl.gov> Co-authored-by: HELICS-bot <HELICS-bot@users.noreply.github.com>
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.
@phlptp hi,
Thank you for these improvements. I have tested it.
I liked the changes you made like (correct me if I'm wrong):
- now all the units target is placed in a namespace
units::units
and this name stays the same no matter wether we buildstatic
orshared
library; - after installation units headers are placed in the directory:
install/units/*.hpp
and thus we include headers as#include <units/units.hpp>
; - generated
units_export.h
is placed ininstall/units/units_export.h
and thus we don't get any errors telling that compiler can't findunits_export.h
. The only thing is thatunits_decl.hpp
includesunits_export.h
as:
#include "units/units_export.h"
but both files units_decl.hpp
and units_export.h
are located in the same folder. I have checked it works fine in any case (#include "units/units_export.h"
or #include "units_export.h"
) but I guess (I don't know exactly) it should be included as #include "units_export.h"
as they are located in the same directory.
Also I have left some meaningful comments on changed files. Please look at them
) | ||
option(UNITS_BUILD_SHARED_LIBRARY "enable Construction of the units shared library" | ||
OFF | ||
) | ||
endif(NOT UNITS_HEADER_ONLY) |
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.
The block:
if(NOT UNITS_HEADER_ONLY)
...
endif(NOT UNITS_HEADER_ONLY)
looks a little hardcoded but I can't propose anything better.
The only thing I can propose is to check whether the developper have checked ON
only one option from these three (UNITS_BUILD_STATIC_LIBRARY , UNITS_BUILD_SHARED_LIBRARY, UNITS_HEADER_ONLY
) and if there are for example both UNITS_BUILD_STATIC_LIBRARY =ON
and UNITS_BUILD_SHARED_LIBRARY=ON
we could send a message (WARNING
or even FATAL_ERROR
) telling that only one library at a time can be built.
Without this it is difficult to predict how the library is going to be built (shared/static/header_only or their combinations) and the developper would need to skim the cmake code to figure that out.
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 added some warnings to check if more than one of UNITS_BUILD_STATIC_LIBRARY, UNITS_BUILD_SHARED_LIBRARY, and UNITS_BUILD_OBJECT_LIBRARY is set, and if you set all three you will get two warnings.
… into modified_cmake_projects
Co-authored-by: Philip Top <top1@llnl.gov> Co-authored-by: HELICS-bot <HELICS-bot@users.noreply.github.com>
… into modified_cmake_projects
@kerim371 I am going to merge this and do a few more things to get an updated release out soon. If you have any more suggestions or tweaks let me know. Thanks |
update the cmake to only allow 1 of the static/shared/object/ header only to be built
Make some tests run against the shared library and fix that up so it actually works properly.