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

CMake Update #768

Merged
merged 1 commit into from
Aug 25, 2022
Merged

CMake Update #768

merged 1 commit into from
Aug 25, 2022

Conversation

phlptp
Copy link
Collaborator

@phlptp phlptp commented Aug 24, 2022

Change the CMake to use subdirectory and move the library generation …and warnings to separate files.

@phlptp
Copy link
Collaborator Author

phlptp commented Aug 24, 2022

pre-commit.ci autofix

@codecov
Copy link

codecov bot commented Aug 24, 2022

Codecov Report

Merging #768 (84aa86d) into main (c7aff0e) will not change coverage.
The diff coverage is n/a.

❗ Current head 84aa86d differs from pull request most recent head a5d2478. Consider uploading reports for the commit a5d2478 to get more accurate results

@@           Coverage Diff           @@
##             main     #768   +/-   ##
=======================================
  Coverage   99.06%   99.06%           
=======================================
  Files          16       16           
  Lines        3964     3964           
=======================================
  Hits         3927     3927           
  Misses         37       37           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@@ -109,10 +109,6 @@ jobs:
gcc8:
containerImage: gcc:8
cli11.std: 17
gcc4.8:
containerImage: gcc:4.8
Copy link
Collaborator

@henryiii henryiii Aug 24, 2022

Choose a reason for hiding this comment

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

Is there a reason this can't just change to helics/buildenv:gcc4-8-builder? Does .ci/azure-cmake.yml break?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My guess would be no, but CMake is already installed so just seemed a waste of time, I will put it back to make sure.

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 put it back in the Docker section, installing CMake takes about 30 seconds on the CI test, so we can reduce the build time slightly by leaving it in its own section, or just leave the extra CMake install in place.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We've got enough jobs where I'm not too worried about it. I'd rather like to move to GHA eventually & then we can probably use the CMake action. Keeping this simple makes it easier to move. ;)

update conan and azure-pipelines to fix gcc 4.8 issue

make the CLI11 target sources only for newer CMake

Change the cmake to use subdirectory and move the library generation and warnings to separate files.
@@ -1,5 +1,5 @@
function(add_cli_exe T)
add_executable(${T} ${ARGN} ${CLI11_headers})
add_executable(${T} ${ARGN})
Copy link
Collaborator

Choose a reason for hiding this comment

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

By removing the headers, doesn't this mean you can't access these in an IDE anymore? (Though if you are using the build-ahead mode, you can - so maybe that's why?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In an IDE, the files are now added to the CLI11 Project, which is new. They are added as build in precompile mode or with newer CMakes they are added as private sources in the interface library.

@henryiii henryiii merged commit faea921 into CLIUtils:main Aug 25, 2022
@henryiii henryiii deleted the cmake_update branch August 25, 2022 12:15
@github-actions github-actions bot added needs changelog Hasn't been added to the changelog yet needs README Needs to be mentioned in the README labels Aug 25, 2022
@henryiii henryiii removed needs changelog Hasn't been added to the changelog yet needs README Needs to be mentioned in the README labels Sep 14, 2022
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.

2 participants