-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
xz_utils: conan v2 support #13038
xz_utils: conan v2 support #13038
Conversation
targets=[target], | ||
build_type=self._effective_msbuild_type, | ||
platforms={"x86": "Win32", "x86_64": "x64"}, | ||
upgrade_project=Version(self.settings.compiler.version) >= "17") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
upgrade_project
logic is lost in this conan v2 migration, I don't know what is the equivalent in the new MSBuild
build helper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and it's required, it fails on Windows with Visual Studio 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
6edbd83
to
2ca71e9
Compare
recipes/xz_utils/all/conanfile.py
Outdated
self.win_bash = True | ||
autotools.configure() | ||
autotools.make() | ||
self.win_bash = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@memsharded could you confirm this is the canonical way to conditionally enable win_bash in conan v2 methods (for this case it's for MinGW, and we ensure that a bash is available on windows build machine by adding msys2
recipe to tool_requires
if user did not define tools.microsoft.bash:path
in its conan configuration)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested locally on windows with MinGW. It works... almost:
It fails during installation because install path is not converted to unix path:
/usr/bin/install -c -m 644 /c/users/spaceim/.conan/data/xz_utils/5.2.5/_/_/build/3cbc13750c61092b2c900ee6caef68cf1cd320b4/src/src/liblzma/api/lzma.h 'C:Usersspaceim.conandataxz_utils5.2.5__package3cbc13750c61092b2c900ee6caef68cf1cd320b4/include/.'
make[4]: Leaving directory '/c/Users/spaceim/.conan/data/xz_utils/5.2.5/_/_/build/3cbc13750c61092b2c900ee6caef68cf1cd320b4/build-release/src/liblzma/api'
make[3]: Leaving directory '/c/Users/spaceim/.conan/data/xz_utils/5.2.5/_/_/build/3cbc13750c61092b2c900ee6caef68cf1cd320b4/build-release/src/liblzma/api'
make[3]: Entering directory '/c/Users/spaceim/.conan/data/xz_utils/5.2.5/_/_/build/3cbc13750c61092b2c900ee6caef68cf1cd320b4/build-release/src/liblzma'
make[4]: Entering directory '/c/Users/spaceim/.conan/data/xz_utils/5.2.5/_/_/build/3cbc13750c61092b2c900ee6caef68cf1cd320b4/build-release/src/liblzma'
/usr/bin/mkdir -p 'C:Usersspaceim.conandataxz_utils5.2.5__package3cbc13750c61092b2c900ee6caef68cf1cd320b4/lib'
/usr/bin/mkdir -p 'C:Usersspaceim.conandataxz_utils5.2.5__package3cbc13750c61092b2c900ee6caef68cf1cd320b4/lib/pkgconfig'
/bin/sh ../../libtool --mode=install /usr/bin/install -c liblzma.la 'C:Usersspaceim.conandataxz_utils5.2.5__package3cbc13750c61092b2c900ee6caef68cf1cd320b4/lib'
/usr/bin/install -c -m 644 liblzma.pc 'C:Usersspaceim.conandataxz_utils5.2.5__package3cbc13750c61092b2c900ee6caef68cf1cd320b4/lib/pkgconfig'
Usage: /c/Users/spaceim/.conan/data/xz_utils/5.2.5/_/_/build/3cbc13750c61092b2c900ee6caef68cf1cd320b4/build-release/libtool [OPTION]... [MODE-ARG]...
Try 'libtool --help' for more information.
libtool: error: 'C:Usersspaceim.conandataxz_utils5.2.5__package3cbc13750c61092b2c900ee6caef68cf1cd320b4/lib' must be an absolute directory name
make[4]: *** [Makefile:876: install-libLTLIBRARIES] Error 1
make[4]: Leaving directory '/c/Users/spaceim/.conan/data/xz_utils/5.2.5/_/_/build/3cbc13750c61092b2c900ee6caef68cf1cd320b4/build-release/src/liblzma'
make[3]: *** [Makefile:1802: install-am] Error 2
make[3]: Leaving directory '/c/Users/spaceim/.conan/data/xz_utils/5.2.5/_/_/build/3cbc13750c61092b2c900ee6caef68cf1cd320b4/build-release/src/liblzma'
make[2]: *** [Makefile:1638: install-recursive] Error 1
make[2]: Leaving directory '/c/Users/spaceim/.conan/data/xz_utils/5.2.5/_/_/build/3cbc13750c61092b2c900ee6caef68cf1cd320b4/build-release/src/liblzma'
make[1]: *** [Makefile:422: install-recursive] Error 1
make[1]: Leaving directory '/c/Users/spaceim/.conan/data/xz_utils/5.2.5/_/_/build/3cbc13750c61092b2c900ee6caef68cf1cd320b4/build-release/src'
make: *** [Makefile:616: install-recursive] Error 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's it. If I replace autotools.install()
by autotools.install(args=[f"DESTDIR={unix_path(self, self.package_folder)}"])
, it works (note: if I call conan create -kb
it fails with ConanException: The config 'tools.microsoft.bash:subsystem' is needed to run commands in a Windows subsystem
, funny since this config is injected by msys2
recipe)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we go: conan-io/conan#12153 & conan-io/conan#12154
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SpaceIm Thank you very much for checking it. The new MSBuild is still limited in terms of features, compared to the legacy.
2ca71e9
to
040e377
Compare
This comment has been minimized.
This comment has been minimized.
Ok, it requires conan >= 1.52.0 :( conan-io/conan#11968 |
I detected other pull requests that are modifying xz_utils/all recipe: This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there. |
This comment has been minimized.
This comment has been minimized.
DESTDIR manually injected due to conan-io/conan#12153
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Ok so the build of xz_utils didn't honor MTd runtime, and it fails afterwards in test_package due to runtime discrepancy. In sln & vcxproj files of xz_utils, there is no /cc @memsharded Actually it's not honored also in MT static build (https://c3i.jfrog.io/c3i/misc/logs/pr/13038/10-configs/windows-visual_studio/xz_utils/5.2.5//7bd6f2c3d5c4e48a75805376b58cde753392f711-test.txt):
|
It should work now (even MinGW). Honestly I'm not very proud of all these MSBuild tricks. There are currently 27 recipes in conan-center relying on legacy MSBuild. If anybody has something more robust to propose (inject conantoolchain.props from MSBuildToolchain, avoid conflicts, honor custom clflags, cxxflags, ldflags from profile etc), please share ! Note: on macos if shared, |
This comment has been minimized.
This comment has been minimized.
variables from https://cmake.org/cmake/help/latest/module/FindLibLZMA.html must be defined LIBLZMA_HAS_AUTO_DECODER, LIBLZMA_HAS_EASY_ENCODER & LIBLZMA_HAS_LZMA_PRESET are not modeled for the moment
This comment has been minimized.
This comment has been minimized.
* conan v2 support * typo * refactor slightly * fix MinGW build DESTDIR manually injected due to conan-io/conan#12153 * workaround to update PlatformToolset in vcxproj files * fix install for compiler=msvc * remove WindowsTargetPlatformVersion in 5.2.4 * add ugly tricks for MSBuild * test custom CMake variables in conan generator variables from https://cmake.org/cmake/help/latest/module/FindLibLZMA.html must be defined LIBLZMA_HAS_AUTO_DECODER, LIBLZMA_HAS_EASY_ENCODER & LIBLZMA_HAS_LZMA_PRESET are not modeled for the moment * typo * set win_bash in build_requirements
* conan v2 support * typo * refactor slightly * fix MinGW build DESTDIR manually injected due to conan-io/conan#12153 * workaround to update PlatformToolset in vcxproj files * fix install for compiler=msvc * remove WindowsTargetPlatformVersion in 5.2.4 * add ugly tricks for MSBuild * test custom CMake variables in conan generator variables from https://cmake.org/cmake/help/latest/module/FindLibLZMA.html must be defined LIBLZMA_HAS_AUTO_DECODER, LIBLZMA_HAS_EASY_ENCODER & LIBLZMA_HAS_LZMA_PRESET are not modeled for the moment * typo * set win_bash in build_requirements
Specify library name and version: lib/1.0
This is also a good place to share with all of us why you are submitting this PR (specially if it is a new addition to ConanCenter): is it a dependency of other libraries you want to package? Are you the author of the library? Thanks!