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

#16427. Make MSBuildDeps generation with deployer relocatable #16441

Conversation

stgatilov
Copy link
Contributor

@stgatilov stgatilov commented Jun 8, 2024

Changelog: Feature: Make MSBuildDeps generation with deployer relocatable.
Docs: Omit

Close #16427

  • 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.

I have copied the relativize_path logic from CMakeDeps, I can't say I fully understand its implementation though.

As far as I understand, paths should be relativized when deployer is present but kept absolute if package is in conan cache. I have not found in the code where this decision happens, but I have tested locally that paths are relative with deployer but absolute without it (even though everything is on the same disk letter).

Please advise where to add test if it is necessary. As far as I see, there are two tests which check that output with deployer is relocatable (tests/functional/command/test_install_deploy.py). If I should make a similar test for MSBuildDeps, where should I put it?

…ed props file when deployer is used.

Copied the logic from conan\tools\cmake\cmakedeps\templates\target_data.py
@CLAassistant
Copy link

CLAassistant commented Jun 8, 2024

CLA assistant check
All committers have signed the CLA.

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.

Thanks for your contribution @stgatilov

This looks good. It would be necessary to add some test to cover this case. I can help adding those tests if you want or feel it is too much, but if you want to try, you can start from https://github.com/conan-io/conan/blob/develop2/test/functional/command/test_install_deploy.py, maybe starting from some test like test_deploy_reference

Note these tests only verify the generated file correctness, but not necessarily use the files in a real build. If you test it also for your full use case, that would be useful, but I don't think it is necessary to add a full building test to the suite to cover this.

@memsharded memsharded added this to the 2.5.0 milestone Jun 8, 2024
…in props with deployer, and absolute paths without deployer.

Note: this test does not invoke msbuild, and thus does not depend on MSVC / Windows.
@stgatilov
Copy link
Contributor Author

I added a simple test.
Full build test would be a bit more reliable, but I guess can't have full-blown tests on every combination of settings/features.

Since my goal was a test without msbuild, the tests in test\integration\toolchains\microsoft\test_msbuilddeps.py were much more useful to me as reference.

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.

The test is looking good enough, it would benefit from some refactors as parameterizing the test instead of a for, but don't worry about it, we ca can do it later.

The only problem is the test is failing in other platforms than Windows:

                    # path should be absolute, since conan cache does not move with project

>                   assert os.path.isabs(value)

E                   AssertionError: assert False

E                    +  where False = <function isabs at 0x7f9e62578a40>('tmp/tmp9yzuhr1lconans/path with spaces/.conan2/p/b/libhb1f8faf955e10/p')

E                    +    where <function isabs at 0x7f9e62578a40> = <module 'posixpath' (frozen)>.isabs

E                    +      where <module 'posixpath' (frozen)> = os.path

It would be perfectly fine to add a @pytest.mark.skipif(platform.system() != "Windows", reason="Requires Windows") to the test so it only runs in WIndows

test/integration/toolchains/microsoft/test_msbuilddeps.py Outdated Show resolved Hide resolved
@memsharded memsharded merged commit 994f271 into conan-io:develop2 Jun 9, 2024
2 checks passed
@memsharded
Copy link
Member

Thanks very much for contributing this!

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.

[bug] MSBuildDeps + deployer generates props with absolute paths
3 participants