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

Modified cmake projects #155

Merged
merged 22 commits into from
Aug 16, 2021
Merged

Modified cmake projects #155

merged 22 commits into from
Aug 16, 2021

Conversation

phlptp
Copy link
Collaborator

@phlptp phlptp commented Aug 12, 2021

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.

@codecov-commenter
Copy link

codecov-commenter commented Aug 12, 2021

Codecov Report

Merging #155 (a03421a) into master (db614a5) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #155   +/-   ##
=======================================
  Coverage   99.25%   99.25%           
=======================================
  Files           6        6           
  Lines        4809     4809           
=======================================
  Hits         4773     4773           
  Misses         36       36           

github-actions bot and others added 2 commits August 12, 2021 15:32
Co-authored-by: Philip Top <top1@llnl.gov>
Co-authored-by: HELICS-bot <HELICS-bot@users.noreply.github.com>
@phlptp phlptp force-pushed the modified_cmake_projects branch 5 times, most recently from b7e48bf to 6ffdb26 Compare August 13, 2021 11:24
@phlptp phlptp force-pushed the modified_cmake_projects branch 8 times, most recently from 413661f to c27430c Compare August 13, 2021 19:08
@phlptp
Copy link
Collaborator Author

phlptp commented Aug 13, 2021

finish up fixes and recommendations from #153 and add tests for the shared library and install packages

@phlptp
Copy link
Collaborator Author

phlptp commented Aug 13, 2021

@kerim371 can you take a look through these changes and see if they match up with your recommendations

phlptp and others added 2 commits August 13, 2021 14:19
Co-authored-by: Philip Top <top1@llnl.gov>
Co-authored-by: HELICS-bot <HELICS-bot@users.noreply.github.com>
Copy link
Contributor

@kerim371 kerim371 left a 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):

  1. now all the units target is placed in a namespace units::units and this name stays the same no matter wether we build static or shared library;
  2. after installation units headers are placed in the directory: install/units/*.hpp and thus we include headers as #include <units/units.hpp>;
  3. generated units_export.h is placed in install/units/units_export.h and thus we don't get any errors telling that compiler can't find units_export.h. The only thing is that units_decl.hpp includes units_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

CMakeLists.txt Show resolved Hide resolved
)
option(UNITS_BUILD_SHARED_LIBRARY "enable Construction of the units shared library"
OFF
)
endif(NOT UNITS_HEADER_ONLY)
Copy link
Contributor

@kerim371 kerim371 Aug 13, 2021

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.

Copy link
Collaborator Author

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.

@phlptp
Copy link
Collaborator Author

phlptp commented Aug 16, 2021

@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

@phlptp phlptp merged commit 15d323a into master Aug 16, 2021
@phlptp phlptp deleted the modified_cmake_projects branch August 16, 2021 20:36
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.

None yet

3 participants