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 pkgconfig name collision on Windows #14428

Open
wants to merge 2 commits into
base: release/2.0
Choose a base branch
from

Conversation

domin144
Copy link

@domin144 domin144 commented Aug 4, 2023

Changelog: (Bugfix): Fix pkgconfig name collision on Windows

This fix is related to #10341. The previous fix did not take into account cases insensitive filesystems. As a result the issue was still present on Windows. OpenEXR 2.x is an example of affected recipe. The "OpenEXR.pc" file for the "openexr_ilmimf" component was overwritten by "openexr.pc" created based on package name.

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

@CLAassistant
Copy link

CLAassistant commented Aug 4, 2023

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 very much for the contribution @domin144

It would be great to start the PR with a failing "red" test failing for this bug, and then see it fixed with this contributed fix. Tests are necessary anyway, so having them from the start help a lot understanding the fixes.

This fix is related to conan-io#10341
The previous fix did not take into account cases insensitive
filesystems. As a result the issue was still present on Windows.
OpenEXR is an example of affected recipe.
@domin144 domin144 force-pushed the feature/fix_windows_pkgconfig_name_collision branch from 3ecaec9 to 80c00bd Compare August 7, 2023 20:30
@domin144
Copy link
Author

domin144 commented Aug 7, 2023

I am having some trouble running the tests. For now I wrote some test, which tries to reproduce an example I tried manually. I did not run the test though. The important lines are:

        self.cpp_info.components["component"].set_property("pkg_config_name", "Hello")
        self.cpp_info.components["component"].libs = ["hello"]

This is where conflicting .pc file comes from. Notice the upper case H i hello. Hello.pc created for this component will be overwritten on Windows with the hello.pc created based on the package name.

When I run the example, I had to add tool_requires = "meson/[>=1.1.0]", "pkgconf/[>=1.9]" to test_package\conanfile.py, but I can see other tests don't to this, so I assume that in the testing environment it is not required. I didn't put this in the test. I attach the example.

BTW: where do you normally get pkg-config on Windows to use with meson + visual studio?

Please bear with me. This is all new to me. I did not use conan before and I don't have much experience with python. I usually don't develop on windows also and hoped conan will make a temporary transition easier for me.

I assume you have a working setup, where you are able to run test. I would appreciate, if you tried to run this test and let me know if it passes with the fix and fails without it.

@memsharded
Copy link
Member

tool_requires = "meson/[>=1.1.0]", "pkgconf/[>=1.9]"

yes, it is not possible to download things from ConanCenter in testing. They need to be installed in the machine, and injected via @pytest.mark.tool("...") annotations, that work with the definition of conftest.py and conftest_user.py

BTW: where do you normally get pkg-config on Windows to use with meson + visual studio?

there is no standard location, this is why we are using the conftest.py and conftest_user.py files to define it.

Please bear with me. This is all new to me. I did not use conan before and I don't have much experience with python. I usually don't develop on windows also and hoped conan will make a temporary transition easier for me.

Yes, thanks very much for your contribution and putting the effort! 🙂 I think @franramirez688 can help the following days with the information you provided in your comment and draft some test that covers it.

@franramirez688
Copy link
Contributor

Hi @domin144 ,

Thanks for raising this problem.

I'm not sure if this is the best way to proceed with this error. Let me talk to the team because I'd like a more generic solution. This one sounds like a breaking change for systems with case-sensitive file systems, so let's see if we could get another approach or even if this is the best one.

@domin144
Copy link
Author

Hi @franramirez688,

I think the OpenEXR recipe tried to reproduce the pc file name, provided by OpenEXR itself (https://github.com/AcademySoftwareFoundation/openexr/blob/d688d05d99dc54f6d928c4e6d6ceee53b3368267/cmake/OpenEXR.pc.in).

Any application prepared to use the original OpenEXR.pc file, will not be broken by absence of the additional openexr.pc file present only when package is provided by conan. It's hard for me to imagine a case, where creating two pc files with names differing only by letter cases is intentional.

With the current, case sensitive match, we have a discrepancy between platforms: on case sensitive file systems two files are created, on case insensitive filesystems the first one is overwritten and only second is available. Case ignoring match makes the procedure consistent.

Anyway, I checked the _get_package_name function and I can see the default pc file name can be overridden, by changing pkg_config_name property of the cpp_info object. This means the issue with OpenEXR could be worked around in the recipe. I will try to make such workaround, but I still think conan should somehow prevent the overwrite of the file or at least provide a warning.

@franramirez688
Copy link
Contributor

Hi @domin144 ,

Thanks for your feedback!

We were checking this issue, and we think that the best approach would have these two points:

  • First, PkgConfigDeps should have a mechanism to define set_property("pkg_config_name", "none") as a placeholder to avoid creating those *.pc files.
  • Then, modify the recipe and add this "none" one to the main OpenEXR's cpp_info to make that possible and avoid overwriting the component one.

We still think that this change would bring breaking changes for several recipes so we prefer to implement those ones instead, and keep that check as case-sensitive.

@domin144
Copy link
Author

Great!
I am happy you have a concept on how to resolve the issue. I am looking forward to the implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants