Skip to content

Make cmake minimum version same accross repo #136

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 31, 2024
Merged

Make cmake minimum version same accross repo #136

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
# Unit tests
# ==========

cmake_minimum_required(VERSION 3.1)
cmake_minimum_required(VERSION 3.24)
Copy link
Contributor

Choose a reason for hiding this comment

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

We do not bump the version of required tools unless there is explicit need for it. Is there such a need here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was just for consistency between the main cmakelists and the one in the tests. No other reason.

Copy link
Collaborator Author

@mcbarton mcbarton May 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It being 3.1 was irrelevant since the main one requested a minimum of 3.24 .

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, then my question holds for the main one but let's see if somebody complains. Generally older tools are available in more places than the newer ones and it is a trade-off between having new tools with better functionality and having them everywhere so that the users can use the software we build.

Copy link
Collaborator Author

@mcbarton mcbarton May 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is where is why the main version was requested to have a new version due the use of MAKE_COMPILE_WARNING_AS_ERROR in the ci #88 (comment) . We could probably reduce the minimum by using a different variable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Regarding consistency - My thinking (also Johan & Sylvain would agree) is that whenever we update the version for some dependency or anything it should enforce consistency among all xeus stack . So technically we look at xeus-python as the first thing where a change is introduced and all other xeus kernels follow.

As far as I know we're using 3.4.3 these days

  1. https://github.com/jupyter-xeus/xeus-python/blob/47cf95cd9ab0e74205ddf80dd8cee92b5d25226d/CMakeLists.txt#L11
  2. https://github.com/jupyter-xeus/xeus-javascript/blob/500ad0c1602a013c2f2cbdc8097da05caef146eb/CMakeLists.txt#L9

cc @vgvassilev @mcbarton

Copy link
Collaborator Author

@mcbarton mcbarton May 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@vgvassilev @anutosh491 So what we need is an older cmake variable which is essentially equivalent to MAKE_COMPILE_WARNING_AS_ERROR . Then we can have a lower minimum which is consistent with the rest of the xeus stack.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that xeus-cpp should do what the rest of the stack does unless it has a good reason not to. For example, treating warnings as errors is a good reason to not conform in my point of view. Of course we can implement it with more code in a conforming way but that’d require more efforts which may not be justified.

I think the change is fine for me, just pointing out something that we should be careful about.


if (CMAKE_CURRENT_SOURCE_DIR STREQUAL CMAKE_SOURCE_DIR)
project(xeus-cpp-test)
Expand Down