Skip to content

Removes usage of cmake IF(IN_LIST), for some env this gives problems.#251

Merged
pnoltes merged 1 commit intomasterfrom
feature/remove_cmake_in_list_usage
Jun 8, 2020
Merged

Removes usage of cmake IF(IN_LIST), for some env this gives problems.#251
pnoltes merged 1 commit intomasterfrom
feature/remove_cmake_in_list_usage

Conversation

@pnoltes
Copy link
Copy Markdown
Contributor

@pnoltes pnoltes commented Jun 7, 2020

The construction if (.. IN_LIST ..) is giving some problems with some version of CMake (If I am correct 3.4 and 3.11).

For that reason this PR replace the usage of IN_LIST with a different approach.

@pnoltes pnoltes requested a review from abroekhuis June 7, 2020 21:56
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Merging #251 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #251      +/-   ##
==========================================
+ Coverage   67.82%   67.84%   +0.01%     
==========================================
  Files         127      127              
  Lines       26375    26375              
==========================================
+ Hits        17890    17894       +4     
+ Misses       8485     8481       -4     
Impacted Files Coverage Δ
libs/utils/src/hash_map.c 94.23% <0.00%> (+1.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f25f19f...22917b4. Read the comment docs.

@abroekhuis
Copy link
Copy Markdown
Contributor

I'm going to object this one. Not sure what the issue is, some details might convinced me otherwise.
But, in_list is already in cmake since 3.3. And more importantly, we should not want to fix down stream issues with such old versions. Some backwards compatibility with older versions is good, but this goes back to far.

@pnoltes
Copy link
Copy Markdown
Contributor Author

pnoltes commented Jun 8, 2020

I'm going to object this one. Not sure what the issue is, some details might convinced me otherwise.
But, in_list is already in cmake since 3.3. And more importantly, we should not want to fix down stream issues with such old versions. Some backwards compatibility with older versions is good, but this goes back to far.

Tested with cmake 3.4 and 3.12 and in both cases the cmake policy CMP0057 has te be set (I expect to NEW) or the build fails.

Normally not a big issues, just set the policy in Celix.
But because the BundlePackaging.cmake is a Celix delivery and used by end users, this means that all users using at least cmake <= 3.12 need to set this policy in their environment or else the build will fail.

@abroekhuis
Copy link
Copy Markdown
Contributor

I'm going to object this one. Not sure what the issue is, some details might convinced me otherwise.
But, in_list is already in cmake since 3.3. And more importantly, we should not want to fix down stream issues with such old versions. Some backwards compatibility with older versions is good, but this goes back to far.

Tested with cmake 3.4 and 3.12 and in both cases the cmake policy CMP0057 has te be set (I expect to NEW) or the build fails.

Normally not a big issues, just set the policy in Celix.
But because the BundlePackaging.cmake is a Celix delivery and used by end users, this means that all users using at least cmake <= 3.12 need to set this policy in their environment or else the build will fail.

3.12 has been release in november 2018, I don't see a big problem here. If it was less then a year ago, perhaps, but not now.

@pnoltes
Copy link
Copy Markdown
Contributor Author

pnoltes commented Jun 8, 2020

I'm going to object this one. Not sure what the issue is, some details might convinced me otherwise.
But, in_list is already in cmake since 3.3. And more importantly, we should not want to fix down stream issues with such old versions. Some backwards compatibility with older versions is good, but this goes back to far.

Tested with cmake 3.4 and 3.12 and in both cases the cmake policy CMP0057 has te be set (I expect to NEW) or the build fails.
Normally not a big issues, just set the policy in Celix.
But because the BundlePackaging.cmake is a Celix delivery and used by end users, this means that all users using at least cmake <= 3.12 need to set this policy in their environment or else the build will fail.

3.12 has been release in november 2018, I don't see a big problem here. If it was less then a year ago, perhaps, but not now.

Note sure why building celix has no issue there, but reading the cmake help:
https://cmake.org/cmake/help/v3.17/policy/CMP0057.html
cmake 3.17 (latest) still set the default value of the policy to old (so ignoring it as an operator value). If I am correct this means that all users need to set this policy to get the Bundle Packaging working.

@abroekhuis
Copy link
Copy Markdown
Contributor

I'm going to object this one. Not sure what the issue is, some details might convinced me otherwise.
But, in_list is already in cmake since 3.3. And more importantly, we should not want to fix down stream issues with such old versions. Some backwards compatibility with older versions is good, but this goes back to far.

Tested with cmake 3.4 and 3.12 and in both cases the cmake policy CMP0057 has te be set (I expect to NEW) or the build fails.
Normally not a big issues, just set the policy in Celix.
But because the BundlePackaging.cmake is a Celix delivery and used by end users, this means that all users using at least cmake <= 3.12 need to set this policy in their environment or else the build will fail.

3.12 has been release in november 2018, I don't see a big problem here. If it was less then a year ago, perhaps, but not now.

Note sure why building celix has no issue there, but reading the cmake help:
https://cmake.org/cmake/help/v3.17/policy/CMP0057.html
cmake 3.17 (latest) still set the default value of the policy to old (so ignoring it as an operator value). If I am correct this means that all users need to set this policy to get the Bundle Packaging working.

Wow, I did not expect that, in that case I don't have a huge argument I guess..
Can it be that one of the included (external) files sets the policy? If so, can't we do the same?

@pnoltes
Copy link
Copy Markdown
Contributor Author

pnoltes commented Jun 8, 2020

I'm going to object this one. Not sure what the issue is, some details might convinced me otherwise.
But, in_list is already in cmake since 3.3. And more importantly, we should not want to fix down stream issues with such old versions. Some backwards compatibility with older versions is good, but this goes back to far.

Tested with cmake 3.4 and 3.12 and in both cases the cmake policy CMP0057 has te be set (I expect to NEW) or the build fails.
Normally not a big issues, just set the policy in Celix.
But because the BundlePackaging.cmake is a Celix delivery and used by end users, this means that all users using at least cmake <= 3.12 need to set this policy in their environment or else the build will fail.

3.12 has been release in november 2018, I don't see a big problem here. If it was less then a year ago, perhaps, but not now.

Note sure why building celix has no issue there, but reading the cmake help:
https://cmake.org/cmake/help/v3.17/policy/CMP0057.html
cmake 3.17 (latest) still set the default value of the policy to old (so ignoring it as an operator value). If I am correct this means that all users need to set this policy to get the Bundle Packaging working.

Wow, I did not expect that, in that case I don't have a huge argument I guess..
Can it be that one of the included (external) files sets the policy? If so, can't we do the same?

Technically I think we can, but I think you should not set a cmake policy for a user. Although in this case a stretch, an end user project can depend on the OLD policy and explicitly set it.

@pnoltes pnoltes merged commit 62e79d4 into master Jun 8, 2020
@pnoltes pnoltes deleted the feature/remove_cmake_in_list_usage branch June 8, 2020 10:57
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.

3 participants