Skip to content
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

Prevent variable overwriting in nested calls to dependency provider #587

Conversation

jcar87
Copy link
Contributor

@jcar87 jcar87 commented Nov 9, 2023

We can have nested calls to find_package, which will be handled by conan_provide_dependency, which is a macro.

Any variable that we set in conan_provide_dependency will be visible by nested calls - if nested calls use the same variable names further on, then the values are rewritten.
As it turns out, this is only causing issues currently as reported here: #570 - note that this PR does not fix that issue.

This PR does not change behaviour - just stylistic changes that make things more evident.

In this PR:

  • Use lowercase local-scope variable names for values retrieved from global properties (stylistic - but avoids confusion with the global properties and enforces the notion that they are local scope variables)
  • unset() some variables right after we're done using them - before any possible nested call to find_package - not that for these variables, everything was working correctly before this PR, but it was coincidence
  • Out of abundance of caution, do not reuse the _find_args variable name, but rather use a variable that is suffixed with the package name being requested - and unset() it immediately after use. The behaviour before this PR was still correct (since the variable was not used after the call to find_package)
  • Only mutate CMAKE_MODULE_PATH if we need to fall back to CMake default search behaviour - note that we're still affected by Setting CMAKE_MODULE_PATH before first find_package() does not restore value #570 and the variable is overwritten if we have two nested calls that end up in the fallback block. This will be soon addressed in https://github.com/conan-io/cmake-conan/pull/571/files#diff-d0a3014e65e75c3075c33559a7b0cccb2baaac5e2d2beb52c7884f5bc15bda5f

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

Looks necessary clean up, good catch

@jcar87 jcar87 marked this pull request as ready for review November 9, 2023 20:16
@memsharded memsharded merged commit 0a466e8 into conan-io:develop2 Nov 9, 2023
4 checks passed
@jcar87 jcar87 deleted the lcc/bugfix/prevent-variable-overwriting-nested-calls branch November 9, 2023 20:58
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.

2 participants