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

libxml2: provide official CMake variables + del fPIC if shared + fix system libs #4625

Merged
merged 13 commits into from
Feb 25, 2021

Conversation

SpaceIm
Copy link
Contributor

@SpaceIm SpaceIm commented Feb 20, 2021

Specify library name and version: lib/1.0

  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Feb 20, 2021

Several modifications are based on the hard work made in #2714 (I've not restored the specific MinGW build but it should be done in a PR or another because shared MinGW is still broken since #2149).

@conan-center-bot
Copy link
Collaborator

All green in build 2 (da7c3e2a84dc76b924add1d5ff8aee9072a1c5eb):

  • libxml2/2.9.9@:
    All packages built successfully! (All logs)

  • libxml2/2.9.10@:
    All packages built successfully! (All logs)

if not self._is_msvc:
if self.options.zlib or self.options.lzma or self.options.icu:
self.build_requires("pkgconf/1.7.3")
if tools.os_info.is_windows and not tools.get_env("CONAN_BASH_PATH"):
Copy link
Contributor

Choose a reason for hiding this comment

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

msys2 is only needed for mingw or am I seeing this wrong? I have often mentioned that this change makes cross compiling on windows for android no longer possible and you install msys2 unnecessarily when using windows clang for example.

Copy link
Contributor Author

@SpaceIm SpaceIm Feb 20, 2021

Choose a reason for hiding this comment

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

This logic was already there (and all recipes where autotools is used for mingw@windows) before this PR:

  1. all triplets will run configure except if Visual Studio compiler
  2. you need a shell to run configure
  3. there is no shell on windows by default
  4. 1+2+3 => we need msys2 if build machine is windows and compiler is not Visual Studio

The reason why it fails for cross compilation doesn't come from this logic, but from conan or msys2 recipe which shouldn't fail in configure() method of msys2 recipe when you cross compile from windows.

A workaround is to set CONAN_BASH_PATH. Anyway you'll need a shell for this recipe if you don't use Visual Studio.

Copy link
Contributor

@dmn-star dmn-star Feb 22, 2021

Choose a reason for hiding this comment

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

This logic was already there

I don't know why cscript/configure.js is limited to VS only on Windows. Actually msys2 is not needed here at all, not even for MinGW.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, it was removed in #2149. I would like to revert this modification but maybe not in this PR since it requires some effort.

Copy link
Contributor

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

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

I think there's a typo

recipes/libxml2/all/conanfile.py Outdated Show resolved Hide resolved
Comment on lines +221 to +222
os.remove(os.path.join(self.package_folder, "lib", "libxml2_a_dll.lib"))
os.remove(os.path.join(self.package_folder, "lib", "libxml2_a.lib" if self.options.shared else "libxml2.lib"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be simplified?

Suggested change
os.remove(os.path.join(self.package_folder, "lib", "libxml2_a_dll.lib"))
os.remove(os.path.join(self.package_folder, "lib", "libxml2_a.lib" if self.options.shared else "libxml2.lib"))
tools.remove_files_by_mask(os.path.join(self.package_folder, "lib"), "libxml2*.lib")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. You remove both libxml2_a.lib and libxml2.lib. One must remain.

@SSE4 SSE4 requested a review from uilianries February 21, 2021 05:26
@conan-center-bot
Copy link
Collaborator

All green in build 4 (e29437ab0148b4e4fd809e6948689e660479dde8):

  • libxml2/2.9.9@:
    All packages built successfully! (All logs)

  • libxml2/2.9.10@:
    All packages built successfully! (All logs)

prince-chrismc
prince-chrismc previously approved these changes Feb 21, 2021
SSE4
SSE4 previously approved these changes Feb 22, 2021
@SpaceIm SpaceIm dismissed stale reviews from SSE4 and prince-chrismc via 900630d February 24, 2021 09:26
@conan-center-bot
Copy link
Collaborator

All green in build 5 (900630d366c75f275fade03fc9ba3d9720a66b57):

  • libxml2/2.9.9@:
    All packages built successfully! (All logs)

  • libxml2/2.9.10@:
    All packages built successfully! (All logs)

@SpaceIm SpaceIm requested a review from SSE4 February 24, 2021 14:29
@conan-center-bot conan-center-bot merged commit ae2b4bc into conan-io:master Feb 25, 2021
@SpaceIm SpaceIm deleted the fix/libxml2-cmake-variables branch February 25, 2021 19:47
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.

7 participants