-
Notifications
You must be signed in to change notification settings - Fork 82
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
[INFRA] Bump CMake for tests #3050
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportBase: 98.21% // Head: 98.21% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #3050 +/- ##
=======================================
Coverage 98.21% 98.21%
=======================================
Files 275 275
Lines 12245 12245
=======================================
+ Hits 12026 12027 +1
+ Misses 219 218 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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.
Tiny questions.
@@ -5,7 +5,7 @@ | |||
# shipped with this file and also available at: https://github.com/seqan/seqan3/blob/master/LICENSE.md | |||
# ----------------------------------------------------------------------------------------------------- | |||
|
|||
cmake_minimum_required (VERSION 3.10) | |||
cmake_minimum_required (VERSION 3.15) |
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.
why 15 here?
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 CMAKE_CXX_COMPILER_LAUNCHER is 3.15, I removed the fallback for pre 3.15
@@ -5,93 +5,28 @@ | |||
# shipped with this file and also available at: https://github.com/seqan/seqan3/blob/master/LICENSE.md | |||
# ----------------------------------------------------------------------------------------------------- | |||
|
|||
cmake_minimum_required (VERSION 3.10) | |||
cmake_minimum_required (VERSION 3.16) |
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.
why not 3.16.9
?
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.
We could do 3.16.3 because that's what Google requires, but we usually don't do patch level. No strong feelings
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.
too quick. We should add a changelog entry
I did check but didn't find a place where we mention the cmake version for the tests. Seqan3 itself still only needs 3.4 (and something higher if you want to install). I'll double check, but I'll put a sentence in the changelog. |
e7b3256
to
3afa88a
Compare
Merging such that tomorrow's cron doesn't run in vain. Will do follow-up PR if something needs to be changed. |
Resolves seqan/product_backlog#422
Resolves #2747