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 config file patching on Windows #3399

Merged
merged 15 commits into from Oct 8, 2018

Conversation

Projects
None yet
4 participants
@weatherhead99
Copy link
Contributor

commented Aug 26, 2018

  • Refer to the issue that supports this Pull Request.
  • If the issue has missing info, explain the purpose/use case/pain/need that covers this Pull Request.
  • I've read the Contributing guide.
  • I've followed the PEP8 style guides for Python code.
  • I've opened another PR in the Conan docs repo to the develop branch, documenting this one. Also adding a description of the changes in the changelog.rst file. https://github.com/conan-io/docs

On Windows, generated cmake config files use unix style paths
(and upper case drive names). Thus the path that gets replaced in
patch_config_files() needs to be manipulated before it will actually
be found in a cmake config file.

fixes: #3398

Changelog: Fix: Paths are replaced correctly on Windows when using CMake().patch_config_files() .
Changelog: Feature: New tool.replace_path_in_file to replace Windows paths in a file doing case-insensitive comparisson and indisctinct path separators comparisson: "/" == "\"

@ghost ghost added the contributor pr label Aug 26, 2018

Show resolved Hide resolved conans/client/build/cmake.py Outdated
Show resolved Hide resolved conans/client/build/cmake.py Outdated
Show resolved Hide resolved conans/client/build/cmake.py Outdated
Fix cmake config file patching on Windows
On Windows, generated cmake config files use unix style paths
(and upper case drive names). Thus the path that gets replaced in
patch_config_files() needs to be manipulated before it will actually
be found in a cmake config file.

@weatherhead99 weatherhead99 force-pushed the weatherhead99:fix_patch_cmake_paths branch from 8fe4a37 to df09e4c Aug 29, 2018

@weatherhead99

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2018

ok, have changed the check to platform.system(), and rebased so it's in one commit

@lasote lasote added this to the 1.8 milestone Aug 30, 2018

@lasote lasote added stage: review and removed contributor pr labels Sep 19, 2018

@ghost ghost assigned lasote Sep 25, 2018

@lasote

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2018

@weatherhead99 I've solved some conflicts. Could you take a look at the failing tests to push it to 1.8? Thanks!

@weatherhead99

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2018

hi @lasote have managed to fix two of the three failures, not sure about the third one yet, will try later.

Show resolved Hide resolved conans/test/build_helpers/cmake_test.py Outdated
Show resolved Hide resolved conans/test/build_helpers/cmake_test.py Outdated
@weatherhead99

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2018

ok, so I think I have all tests passing now, but there is a bit of a kludge here:
I'm making the tests pass by re-using the formatting logic that I used in the patch_config_paths function itself.
Since the tests don't actually call cmake with an export() statement, they don't test whether this formatting logic itself is correct. I'm not familiar enough with the conan testing framework to know how to write a test that actually exercises this... any suggestions?

Could we add a suitable export() logic to whatever CMakeLists.txt file actually gets involved when we use TestClient?

@lasote

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2018

Hi, I've opened a PR in your fork. Let me know what you think.

weatherhead99#6

@@ -12,7 +12,7 @@
from conans.client.tools.oss import detected_architecture, os_info
from conans.errors import ConanException
from conans.util.env_reader import get_env
from conans.util.files import decode_text, save, mkdir_tmp
from conans.util.files import decode_text, save, mkdir_tmp, load

This comment has been minimized.

Copy link
@lasote

lasote Sep 28, 2018

Contributor

Sorry, I forgot to remove this unused import

This comment has been minimized.

Copy link
@lasote

lasote Sep 28, 2018

Contributor

Some tests failing, I'll reopen the pull request fixing everything.

lasote and others added some commits Sep 28, 2018

@danimtb

danimtb approved these changes Oct 1, 2018

@ghost ghost assigned memsharded Oct 8, 2018

@lasote lasote merged commit c12a7b7 into conan-io:develop Oct 8, 2018

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
license/cla Contributor License Agreement is signed.
Details

@ghost ghost removed the stage: review label Oct 8, 2018

grisumbras pushed a commit to grisumbras/conan that referenced this pull request Dec 27, 2018

Fix cmake config file patching on Windows (conan-io#3399)
* Fix cmake config file patching on Windows

On Windows, generated cmake config files use unix style paths
(and upper case drive names). Thus the path that gets replaced in
patch_config_files() needs to be manipulated before it will actually
be found in a cmake config file.

* attempt to fix tests on windows

* remove unneeded print & pass statements

* fix final test under windows

* 2nd attempt at final test

* Different approach

* Better replace_in_file

* Removed dead code

* Extracted to replace path in file

* Fixed tests

* unused import
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.