-
Notifications
You must be signed in to change notification settings - Fork 362
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
Adding CMake 3.4 check #394
Conversation
c4cec06
to
a0a976a
Compare
Codecov Report
@@ Coverage Diff @@
## master #394 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 12 12
Lines 3565 3580 +15
=====================================
+ Hits 3565 3580 +15
Continue to review full report at Codecov.
|
3ab9f65
to
a864f65
Compare
69e86d0
to
e53cd0f
Compare
e53cd0f
to
f3e954d
Compare
f3e954d
to
3e90140
Compare
@all-contributors please add @slurps-mad-rips for packaging |
I couldn't determine any contributions to add, did you specify any contributions? |
@all-contributors please add @slurps-mad-rips for platform work |
I've put up a pull request to add @slurps-mad-rips! 🎉 |
- name: Build | ||
run: cmake --build build -j2 | ||
|
||
cmake-config: |
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.
how long does this take to run on all versions of cmake?
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 think I want to try this out on my windows system tonight just to make sure it all works correctly. I like most of the changes, I agree the heavy use of generator expressions may or may not be desirable. |
Please do, most importantly, I don't want to break anything. |
If the CLI11_INSTALL is set to OFF I get
I am guessing somewhere something is not acknowledging that flag |
Ok, ran it through a bunch of different configurations on windows and MSYS. The CLI11_INSTALL=OFF error should be fixed or the option removed before merging though. Longer term we may want to consider making the gtest options hidden so they don't even show up on the cmake-gui interface. I think there might be a few things that need tweaking once CLI11 moves to have a compiled component(or at least the option of one) |
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 CLI11_INSTALL=OFF error should be fixed or the option removed.
For what its worth, older versions of CMake always add their items to the (Also, sorry for dropping off the face of the earth for a bit. Life has been getting in the way. Thanks @henryiii for taking care of this |
FWIW, macOS 10.14 + C++17 mode is broken:
And the warnings on Windows for C++17 mode. Not related to this PR, though. |
52ab464
to
c137fbf
Compare
Reimplements and closes #321. Changes:
CLI11_CXX_STD
->CMAKE_CXX_STANDARD
(only changed if set)CLI11_BUILD_EXAMPLES_JSON
asks for json example