Fixes #570: Do not remove CMAKE_MODULE_PATH if already set on find_package() #571

curlybeast · 2023-10-04T17:46:49Z

This fixes the issue for me.

Close #570

CLAassistant · 2023-10-04T17:46:54Z

All committers have signed the CLA.

memsharded

Thanks for your contribution @curlybeast

I don't fully get what is the difference, looks similar behavior to me.
Maybe having a unit test that would fail without the change would clarify it (and a test seems necessary anyway). Do you think you could try to contribute a test too? If not, don't worry, just tell us, and we will try to help with that.

UPDATE: Sorry I hadn't seen #570, I have added a reference in this PR message.

I know understand the reasons. Still, that further reveals the importance of adding a test to cover this test for now and to avoid future breakage.

curlybeast · 2023-10-05T09:20:52Z

I don't fully get what is the difference, looks similar behavior to me.
The CMAKE_MODULE_PATH is a list. All this patch does is:

Only add the provider path once.
Only remove the provider path if it's in the list.

Using strings as before means that subsequent runs of that function may change the originally saved string irrevocably, which I believe is what is happening.

Do you think you could try to contribute a test too? If not, don't worry, just tell us, and we will try to help with that.
If I find time. But I've also not used the test framework you're using. Would appreciate some help with that if possible.
I agree a test should be added.

curlybeast · 2023-10-05T10:16:06Z

I've added the beginnings of a test (to the same branch / PR) which replicate the find_package() calls I'm using in my project.
I am using REQUIRED though and we can't really do this. I also can not reproduce the problem (if I revert the fix) using this test, which is odd. I wonder if one of the modules (my hunch is the OpenSSL module) is doing another find_package() that triggers this?

If that is the problem, the question is how to test this effectively without requiring a bunch of pre-installed libraries. Thoughts?

memsharded · 2023-10-05T12:18:30Z

I've added the beginnings of a test (to the same branch / PR) which replicate the find_package() calls I'm using in my project.
I am using REQUIRED though and we can't really do this. I also can not reproduce the problem (if I revert the fix) using this test, which is odd. I wonder if one of the modules (my hunch is the OpenSSL module) is doing another find_package() that triggers this?

This is good, exactly the reason why adding the tests are important :)

Indeed do not depend on external packages if possible

It is possible to create simple packages locally, that simulate the openssl package behavior, with an internal find_package or whatever (when using CMakeDeps, this will not happen, not sure about your setup, if you were using openssl in a different way, not via CMakeDeps).

memsharded · 2023-10-05T12:16:33Z

tests/resources/preserve_module_path/CMakeLists.txt

+add_executable(app main.cpp)
+target_link_libraries(app hello::hello bye::bye)


The executable build can be removed for the test

memsharded · 2023-10-05T12:16:42Z

tests/test_smoke.py

+        run("cmake .. -DCMAKE_PROJECT_TOP_LEVEL_INCLUDES=conan_provider.cmake -DCMAKE_BUILD_TYPE=Release Module -DCMAKE_C_COMPILER=/usr/bin/cc", check=False)
+        out, _ = capfd.readouterr()
+        assert "We should be seeing this warning, if we do the test works" in out
+        run("cmake --build .")


This line an be removed

memsharded · 2023-10-05T12:18:03Z

tests/resources/preserve_module_path/CMakeLists.txt

+find_program(CCACHE ccache)
+find_package(OpenSSL)
+find_package(ZLIB)
+find_package(prometheus-cpp)


This would be the thing to simplify, maybe adding a fake openssl-config.cmake in another folder and adding it to the CMAKE_PREFIX_PATH?

Yea, these are the find_package() calls before the problem occurs.
I'll do some digging in my environment to pinpoint exactly what it is.

… find_package()

jcar87 · 2023-11-13T16:32:59Z

Hi @curlybeast - I've sync'ed up the changes with the develop2 branch, and rebased accordingly. I'll take care of the tests as well, as there are a few more things ! Keep this in mind, if you work on this branch locally you'll have to do git fetch followed by git reset --hard origin/develop, otherwise you'll may get merge errors.

jcar87

LGTM

Thanks @curlybeast - the issue happened in a very specific case where two nested find_package calls were satisfied by a CMake FIND module, rather than a config. Only in that scenario was CMAKE_MODULE_PATH incorrectly restored. Your solution fixes this.

We discovered a second issue - some recipes can define cpp_info.builddirs, which result in paths being prepended to CMAKE_MODULE_PATH - those were being lost too. Happy to report your proposed solution also fixes that bug :)

I've added tests for both situations.

curlybeast · 2023-11-14T08:42:48Z

Thank you for the help on this @jcar87, glad to hear I could help the project and that we found / fixed other related issues too! :)

memsharded reviewed Oct 5, 2023

View reviewed changes

curlybeast added 2 commits November 13, 2023 16:04

Fixes conan-io#570: Do not remove CMAKE_MODULE_PATH if already set on…

782fe79

… find_package()

Test conan-io#570: Initial test case, but not working yet

cf65462

jcar87 force-pushed the develop2 branch from 8826efc to cf65462 Compare November 13, 2023 16:31

jcar87 added 4 commits November 13, 2023 20:37

Fix variable name

84fa3c8

Add test for CMake module path mutation and refactor resources

d5e49cd

Remove unused resources

41a2ddd

Add test to ensure builddirs from package_info is respected

4971899

jcar87 approved these changes Nov 13, 2023

View reviewed changes

test: handle multi-config generator

44903f8

jcar87 merged commit c35579a into conan-io:develop2 Nov 14, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #570: Do not remove CMAKE_MODULE_PATH if already set on find_package() #571

Fixes #570: Do not remove CMAKE_MODULE_PATH if already set on find_package() #571

curlybeast commented Oct 4, 2023 •

edited by memsharded

Loading

CLAassistant commented Oct 4, 2023 •

edited

Loading

memsharded left a comment •

edited

Loading

curlybeast commented Oct 5, 2023

curlybeast commented Oct 5, 2023

memsharded commented Oct 5, 2023

memsharded Oct 5, 2023

memsharded Oct 5, 2023

memsharded Oct 5, 2023

curlybeast Oct 5, 2023

jcar87 commented Nov 13, 2023

jcar87 left a comment

curlybeast commented Nov 14, 2023

		add_executable(app main.cpp)
		target_link_libraries(app hello::hello bye::bye)

Fixes #570: Do not remove CMAKE_MODULE_PATH if already set on find_package() #571

Fixes #570: Do not remove CMAKE_MODULE_PATH if already set on find_package() #571

Conversation

curlybeast commented Oct 4, 2023 • edited by memsharded Loading

CLAassistant commented Oct 4, 2023 • edited Loading

memsharded left a comment • edited Loading

Choose a reason for hiding this comment

curlybeast commented Oct 5, 2023

curlybeast commented Oct 5, 2023

memsharded commented Oct 5, 2023

memsharded Oct 5, 2023

Choose a reason for hiding this comment

memsharded Oct 5, 2023

Choose a reason for hiding this comment

memsharded Oct 5, 2023

Choose a reason for hiding this comment

curlybeast Oct 5, 2023

Choose a reason for hiding this comment

jcar87 commented Nov 13, 2023

jcar87 left a comment

Choose a reason for hiding this comment

curlybeast commented Nov 14, 2023

curlybeast commented Oct 4, 2023 •

edited by memsharded

Loading

CLAassistant commented Oct 4, 2023 •

edited

Loading

memsharded left a comment •

edited

Loading