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 - cannot build with 'msvc' compiler #13511

Merged
merged 35 commits into from
Nov 23, 2022

Conversation

paulharris
Copy link
Contributor

I've read over and over that the "Visual Studio" compiler is going away in v2,
to be replaced with the "msvc" compiler.

So I thought I'd switch to "msvc" now while I was in Windows and upgrading to v2.

libxml2 won't build as it does this:

            args = [
                "cscript",
                "configure.js",
                "compiler=msvc",
                "prefix={}".format(self.package_folder),
                # THIS is completely wrong for msvc:
                # - it will append cruntime=/dynamic instead of cruntime=/MD
                # so this fails for static or dynamic builds on windows for the 'msvc' compiler.
                "cruntime=/{}".format(self.settings.compiler.runtime),
                "debug={}".format(debug),
                "static={}".format(static),
            ]

ie one fundamental difference in settings.yml for VS and msvc is the "runtime" specification.
There are LOTS of scripts that are assuming the runtime will equal either "MD" or "MT",
but there doesn't appear to be any guidance in the Migration documentation, or any checks in linter...
Is there any CI machines building for "msvc" ?

Attached is the build log for libxml2 for the "msvc" compiler, and "Visual Studio (vs)" compiler.
The main difference appears to me that the recipe calls cl.exe with /dynamic instead of /MD... and /dynamic is not understood.

I'm not sure what to do about this recipe, but surely we should be building and testing with the "msvc" compiler for the v2 migration now ?

build-vs.log
build-msvc2.log

@conan-center-bot

This comment has been minimized.

@paulharris
Copy link
Contributor Author

I added some code to the recipe to translate "dynamic + Release" runtime to "MD", and then tried to build.
This time it failed, but I cannot see why.

See log file attached, it is really weird that cl.exe complains about -dynamic but there is no mention on the cl.exe command line.
I built for "Visual Studio" and "msvc", and diff'd the conan build folders... there is almost no difference at all ... just different dependency folder paths.

So it looks like -dynamic is coming from conan itself during the calls? Or in the environment somehow?

buildd-msvc.log

@conan-center-bot

This comment has been minimized.

@paulharris
Copy link
Contributor Author

We still need to fixup msvc support, however after that:

libxml2 upstream has CMake support now, although quote "mainly for Windows"
https://gitlab.gnome.org/GNOME/libxml2

The CMake file has been available since around 2.9.11 I think, so we could drop some old libxml2 versions and delete the MSVC part of the recipe.
Although since support is still new, I'd probably prefer to drop 2.9.x and aim for a hopefully more mature cmake build.
Perhaps it works for non-windows and we can drop the AutoTools part of the recipe too?

@jwillikers
Copy link
Contributor

@paulharris I'm not sure why msvc isn't tested in CI. That would be nice, I've had to fix recipes specifically for msvc.

Would conan.tools.microsoft.msvc_runtime_flag help here?

The DBus recipe has transitioned over time from Autotools to CMake to CMake for older versions and Meson for the latest version. I think adding CMake for the releases that support it wouldn't be a bad idea if it works well, even if it's only for Windows. Maybe msvc could be marked as unsupported in the validate method for the older versions which only support Autotools on Windows?

@paulharris
Copy link
Contributor Author

@paulharris I'm not sure why msvc isn't tested in CI. That would be nice, I've had to fix recipes specifically for msvc.

Would conan.tools.microsoft.msvc_runtime_flag help here?

The DBus recipe has transitioned over time from Autotools to CMake to CMake for older versions and Meson for the latest version. I think adding CMake for the releases that support it wouldn't be a bad idea if it works well, even if it's only for Windows. Maybe msvc could be marked as unsupported in the validate method for the older versions which only support Autotools on Windows?

Uh I think there is a misunderstanding:

  • the older versions all support nmake/visualstudio (CMake is the new tool of choice but not sure whether it works for Linux - upstream didn't mention in their readme). So no need to mark as unsupported in validate.
  • I am using msvc_runtime_flag(), but the failure still happens. And the -dynamic flag appears out of nowhere, it feels like the old nmake helper is helpfully injecting that flag into the environment somehow.

Currently I've reverted back to building everything with the Visual Studio conan-compiler. There are a bunch of recipes that obviously need msvc supported added, and I'm not sure how. Just tweaking them to use is_msvc() and msvc_runtime_flag() doesn't work in this case, and I'm not sure why.

@SpaceIm pointed out this issue: conan-io/conan#12188
I think this problem is a potential v2 blocker...

@paulharris
Copy link
Contributor Author

So, the problem still remains...
even with the msvc_runtime_flag() I still get -dynamic flags added to the compiler cmd BUT only for the "msvc" compiler. The "Visual Studio" compiler works fine.

Log files here:

This one works, but with the old "Visual Studio" compiler definition
log-1-vs.txt

This one fails with -dynamic parameter warnings, and link errors for test_package.
This uses the new "msvc" compiler.
log-1-msvc.txt

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@paulharris paulharris marked this pull request as draft October 26, 2022 06:37
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@paulharris
Copy link
Contributor Author

(Note I have disabled msvc while this is debugged)

I need some help with this... mingw acting weird.
I've stripped out the old configure.js stuff and just using autotools directly.
All looking good, the library builds fine.
But, if I build the "utils" (other .exes in the source - same subfolder too) then it fails.

Works fine on Linux, but on mingw, all of the internal flags (like -DHAVE_CONFIG_H) seems to be stripped out of the compile call.

Here it is building on Proof:
https://github.com/eirikb/proof-of-conan/actions/runs/3327117423/jobs/5501551304

this is for the broken mingw call, notice the dep includes are there ...

gcc -m64 -O3 -s -DNDEBUG -I/k/env/conandata/libiconv/1.17/_/_/package/7ad7ee0900bac91afcfcfe22f992c5a3968df519/include -I/k/env/conandata/zlib/1.2.13/_/_/package/7ad7ee0900bac91afcfcfe22f992c5a3968df519/include -m64 -L/k/env/conandata/libiconv/1.17/_/_/package/7ad7ee0900bac91afcfcfe22f992c5a3968df519/lib -L/k/env/conandata/zlib/1.2.13/_/_/package/7ad7ee0900bac91afcfcfe22f992c5a3968df519/lib  /k/env/conandata/libxml2/2.9.14/_/_/build/b31ca6b98fb8ac68b831393ca197790b8ab5b127/src/xmllint.c   -o xmllint


this is a gcc call for a regular library source file, same build, notice a lot  more flags:

 gcc -DHAVE_CONFIG_H -I. -I/k/env/conandata/libxml2/2.9.14/_/_/build/b31ca6b98fb8ac68b831393ca197790b8ab5b127/src -I./include -I/k/env/conandata/libxml2/2.9.14/_/_/build/b31ca6b98fb8ac68b831393ca197790b8ab5b127/src/include -DNDEBUG -I/k/env/conandata/libiconv/1.17/_/_/package/7ad7ee0900bac91afcfcfe22f992c5a3968df519/include -I/k/env/conandata/zlib/1.2.13/_/_/package/7ad7ee0900bac91afcfcfe22f992c5a3968df519/include -pedantic -Wall -Wextra -Wshadow -Wpointer-arith -Wcast-align -Wwrite-strings -Waggregate-return -Wstrict-prototypes -Wmissing-prototypes -Wnested-externs -Winline -Wredundant-decls -Wno-long-long -Wno-format-extra-args -D_REENTRANT -m64 -O3 -s -MT xmlmodule.lo -MD -MP -MF .deps/xmlmodule.Tpo -c /k/env/conandata/libxml2/2.9.14/_/_/build/b31ca6b98fb8ac68b831393ca197790b8ab5b127/src/xmlmodule.c -o xmlmodule.o



This is the failing gcc call, on linux, where it works - note all of the 'internal' flags are there (with a few extras as i had lzma enabled):

/usr/lib/ccache/gcc-11 -DHAVE_CONFIG_H -I. -I/build/conandata/conan-data/libxml2/2.9.14/_/_/build/533d5b85128f28db56a2a9264a32a3ae57ac032c/src  -I./include -I/build/conandata/conan-data/libxml2/2.9.14/_/_/build/533d5b85128f28db56a2a9264a32a3ae57ac032c/src/include -DNDEBUG -pedantic -Wall -Wextra -Wshadow -Wpointer-arith -Wcast-align -Wwrite-strings -Waggregate-return -Wstrict-prototypes -Wmissing-prototypes -Wnested-externs -Winline -Wredundant-decls -Wno-long-long -Wno-format-extra-args -D_REENTRANT -I/build/conandata/conan-data/zlib/1.2.13/_/_/package/0582ee0920dad9fef631620d3ef80b963208d3a1/include  -I/build/conandata/conan-data/xz_utils/5.2.5/_/_/package/0582ee0920dad9fef631620d3ef80b963208d3a1/include -DLZMA_API_STATIC  -m64 -fPIC -O3 -s -MT xmllint.o -MD -MP -MF .deps/xmllint.Tpo -c -o xmllint.o /build/conandata/conan-data/libxml2/2.9.14/_/_/build/533d5b85128f28db56a2a9264a32a3ae57ac032c/src/xmllint.c

I built it again on linux and windows-mingw with the same options and diff'd the build folder, attached is result.
buggy_libxml2-diff-linux-to-mingw.txt

@paulharris
Copy link
Contributor Author

@madebr I hear you might be able to help?

@SpaceIm
Copy link
Contributor

SpaceIm commented Oct 26, 2022

Do not drop MinGW specific branching, it has already been tried #2149, it was broken #2708, and fixed to restore this specific call to configure.js instead of autotools for MinGW #7108.

@paulharris
Copy link
Contributor Author

Wow that is a lot of attempts.

Well, what if we switched to using its cmake builder?
https://gitlab.gnome.org/GNOME/libxml2/-/blob/master/CMakeLists.txt

@paulharris
Copy link
Contributor Author

I converted all the "{}".format(abc) style strings to f"{abc}" strings,
some of them were simple but others were more complex, eg join calls etc.

I normally only convert the very simple strings, and if there is a simple string followed by a complex one that is symmetrical, I normally would leave them both as a .format() so you can read the code easier and compare simple-and-complex.

Is there a general policy on this? What do you guys want to see?

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@prince-chrismc prince-chrismc mentioned this pull request Nov 4, 2022
4 tasks
@conan-center-bot

This comment has been minimized.

@paulharris paulharris closed this Nov 6, 2022
@paulharris paulharris reopened this Nov 6, 2022
@paulharris
Copy link
Contributor Author

The CI gave me a weird error message, so I'm retrying. Not sure if it is a CI problem, or my problem...

C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\MSBuild\Microsoft\VC\v160\Microsoft.CppCommon.targets(1074,5): error MSB6003: The specified task executable "link.exe" could not be run. System.IO.DirectoryNotFoundException: Could not find a part of the path 'C:\J\w\prod\BuildSingleReference@2\conan-center-index\recipes\libxml2\all\test_v1_cmake_module_package\build\2f3c911d3a03a350e59795da872b8587e21687a2\test_cmake_module_package\test_package.dir\Release\test_package.tlog'. [C:\J\w\prod\BuildSingleReference@2\conan-center-index\recipes\libxml2\all\test_v1_cmake_module_package\build\2f3c911d3a03a350e59795da872b8587e21687a2\test_cmake_module_package\test_package.vcxproj]

https://c3i.jfrog.io/c3i/misc/logs/pr/13511/27-configs/windows-visual_studio/libxml2/2.9.12//01aaa5e96f27929fb6bba2fc58f6ed7aff9db697-test.txt

@conan-center-bot

This comment has been minimized.

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline

All green in build 29 (8f33399c4a1683c4bea03fa10d52550f22c1eb08):

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

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

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

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

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

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

@paulharris
Copy link
Contributor Author

@prince-chrismc heads-up: the technique of using add_subdirectory() to the v1 test didn't work for this package,
see above - Failure #28, before my commit 8f33399

Anyway, the recipe now passes :) Ready for review!

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.

This is a huge lift, if there are small details then we can save them for a follow up PR - lets get this out and see the feedback

Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

LGTM

@conan-center-bot conan-center-bot merged commit 4b2a41a into conan-io:master Nov 23, 2022
Comment on lines +6 to +12
message("LIBXML2_FOUND: ${LIBXML2_FOUND}")
message("LIBXML2_INCLUDE_DIR: ${LIBXML2_INCLUDE_DIR}")
message("LIBXML2_INCLUDE_DIRS: ${LIBXML2_INCLUDE_DIRS}")
message("LIBXML2_LIBRARIES: ${LIBXML2_LIBRARIES}")
message("LIBXML2_LIBRARY: ${LIBXML2_LIBRARY}")
message("LIBXML2_DEFINITIONS: ${LIBXML2_DEFINITIONS}")
message("LIBXML2_VERSION_STRING: ${LIBXML2_VERSION_STRING}")
Copy link
Contributor

Choose a reason for hiding this comment

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

it's not a test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was there before, and things that use it as a module tend to want these defined,
so I was checking with these.
Happy for them to be removed in a future PR :)

@paulharris paulharris deleted the libxml2-link-error branch November 25, 2022 15:54
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.

6 participants