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

fix CMAKE_MODULE_PATH env var #19

Merged
merged 1 commit into from
Jun 20, 2018
Merged

fix CMAKE_MODULE_PATH env var #19

merged 1 commit into from
Jun 20, 2018

Conversation

dirk-thomas
Copy link
Member

Currently the environment hooks are being generated in (depending in where the CMake module was found):

<prefix>/share/<pkgname>/cmake/Modules/share/<pkgname>/hook/cmake_module_path.bat

and the appended path points to:

<prefix>

With this patch the location of the environment hook is not nested anymore:

<prefix>/share/<pkgname>/hook/cmake_module_path.bat

and the appended path is the directory containing the CMake module:

<prefix>/share/<pkgname>/cmake/Modules

The first fix is just cosmetic. The second one avoid cross-talk between packages (one extending the module path and another one benefiting from it).

Additionally the patch fixes the case where a package installs multiple CMake module into different location. Before the last hook was overwriting all previous ones.

@dirk-thomas dirk-thomas added bug Something isn't working review Waiting for review (Kanban column) labels Jun 19, 2018
@dirk-thomas dirk-thomas added this to the 0.2.2 milestone Jun 19, 2018
@dirk-thomas dirk-thomas self-assigned this Jun 19, 2018
@dirk-thomas
Copy link
Member Author

In a non-isolated install this bug also results in numerous extra environment hooks to be created.

Copy link

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

lgtm with one question

'cmake_module_path', prefix_path / dirpath,
pkg_name, 'CMAKE_MODULE_PATH', '', mode='prepend')
'cmake_module_path' +
(str(count) if count > 1 else ''),
Copy link

Choose a reason for hiding this comment

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

I don't fully understand why this needs to have a number appended every time, won't you generate a lot of redundant environment hooks if there are several find modules in the same location?

Copy link
Member Author

Choose a reason for hiding this comment

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

The outer loop iterates over different directories and the inner loop iterates over the files in one directory. For each directory only one hook can be generated (hence a break after the generation in the inner loop). For a different directory another hook can be generated though which would point to a different directory.

To make an example for a package foo providing CMake modules in two different locations there would now be two hooks which prepend two different directories to CMAKE_MODULE_PATH:

  • hook cmake_module_path.sh would prepend <prefix>/share/foo/some/location (even if there are multiple CMake modules in this directory only one hook is generated)
  • hook cmake_module_path2.sh would prepend <prefix>/share/foo/other/location

Does that make sense?

Copy link

Choose a reason for hiding this comment

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

Yes 👍

@dirk-thomas dirk-thomas merged commit 0228e97 into master Jun 20, 2018
@dirk-thomas dirk-thomas deleted the fix_cmake_module_path branch June 20, 2018 04:21
@dirk-thomas dirk-thomas removed the review Waiting for review (Kanban column) label Jun 20, 2018
@dirk-thomas
Copy link
Member Author

So the idea was good and it works well in isolated installations. But...

In the --merge-install case this is still a problem. After a package A installed its CMake module (and got the intended hook for it) a later processed package B will find the same CMake module and also generate a hook for it. While the presence of the extra hook doesn't do any harm to the environment (since the entry is already there it will be skipped) it does add a lot of unnecessary hooks which takes extra time for sourcing the setup files.

I think the only "reliable" option is to limit the search to location which are considered by CMake as well as contain the package name in the path. I will create a follow up PR for this after confirming this works for our cases...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging this pull request may close these issues.

2 participants