Skip to content

[tinyxml2] Update to 8.0.0; avoid exporting symbols when building static libraries#11616

Merged
dan-shaw merged 2 commits intomicrosoft:masterfrom
orudge:tinyxml2-fixed-exports
Jun 12, 2020
Merged

[tinyxml2] Update to 8.0.0; avoid exporting symbols when building static libraries#11616
dan-shaw merged 2 commits intomicrosoft:masterfrom
orudge:tinyxml2-fixed-exports

Conversation

@orudge
Copy link
Contributor

@orudge orudge commented May 27, 2020

This PR updates tinyxml2 to version 8.0.0. It also adds a patch to prevent symbols being exported from DLLs/EXEs linked with the static version of the library. (This patch has been submitted to leethomason/tinyxml2#820 and hopefully will make it into the next release, so should only be temporary in vcpkg.)

I've tested builds of x86-windows, x86-windows-static, x64-windows, x64-windows-static, arm64-windows, arm64-windows-static. Symbols are still exported from the -windows DLLs correctly, and linked output of -windows-static no longer includes tinyxml2 symbols. Also tested x64-linux which looks to be fine.

@NancyLi1013 NancyLi1013 self-assigned this May 28, 2020
@NancyLi1013 NancyLi1013 added category:port-update The issue is with a library, which is requesting update new revision requires:author-response labels May 28, 2020
@orudge orudge force-pushed the tinyxml2-fixed-exports branch from c8f29bc to 89ace06 Compare May 28, 2020 13:05
@NancyLi1013
Copy link
Contributor

@orudge
Could you please look into the regression on osx?

CMake Error in src/CMakeLists.txt:
  Imported target "TINYXML2::TINYXML2" includes non-existent path

    "/Volumes/data/work/1/s/packages/tinyxml2_x64-osx/debug/include"

  in its INTERFACE_INCLUDE_DIRECTORIES.  Possible reasons include:

  * The path was deleted, renamed, or moved to another location.

  * An install or uninstall procedure did not complete successfully.

  * The installation package was faulty and references files it does not
  provide.

@orudge
Copy link
Contributor Author

orudge commented Jun 1, 2020

Could you please look into the regression on osx?

From what I can tell, this doesn't seem to be a regression (at least, not caused by my changes). Using a clean clone of Microsoft/vcpkg, with existing tinyxml2, I get the same error:

$ ./vcpkg install ignition-msgs5
Computing installation plan...
The following packages will be built and installed:
  * eigen3[core]:x64-osx
  * ignition-cmake2[core]:x64-osx
  * ignition-math6[core]:x64-osx
  * ignition-modularscripts[core]:x64-osx
    ignition-msgs5[core]:x64-osx
  * protobuf[core]:x64-osx
  * tinyxml2[core]:x64-osx

...

Starting package 7/7: ignition-msgs5:x64-osx
Building package ignition-msgs5[core]:x64-osx...
-- Downloading https://github.com/ignitionrobotics/ign-msgs/archive/ignition-msgs5_5.1.0.tar.gz...
-- Extracting source /Users/orudge/vcpkg/vcpkg/downloads/ignitionrobotics-ign-msgs-ignition-msgs5_5.1.0.tar.gz
-- Applying patch 01-protobuf.patch
-- Using source at /Users/orudge/vcpkg/vcpkg/buildtrees/ignition-msgs5/src/sgs5_5.1.0-8f48d4cbaf
-- Configuring x64-osx-dbg
CMake Error at scripts/cmake/vcpkg_execute_required_process.cmake:72 (message):
    Command failed: /Users/orudge/vcpkg/vcpkg/downloads/tools/cmake-3.17.2-osx/cmake-3.17.2-Darwin-x86_64/CMake.app/Contents/bin/cmake /Users/orudge/vcpkg/vcpkg/buildtrees/ignition-msgs5/src/sgs5_5.1.0-8f48d4cbaf -DBUILD_TESTING=OFF -DCMAKE_MAKE_PROGRAM=/Users/orudge/vcpkg/vcpkg/downloads/tools/ninja-1.10.0-osx/ninja -DCMAKE_SYSTEM_NAME=Darwin -DBUILD_SHARED_LIBS=OFF -DVCPKG_CHAINLOAD_TOOLCHAIN_FILE=/Users/orudge/vcpkg/vcpkg/scripts/toolchains/osx.cmake -DVCPKG_TARGET_TRIPLET=x64-osx -DVCPKG_SET_CHARSET_FLAG=ON -DVCPKG_PLATFORM_TOOLSET=external -DCMAKE_EXPORT_NO_PACKAGE_REGISTRY=ON -DCMAKE_FIND_PACKAGE_NO_PACKAGE_REGISTRY=ON -DCMAKE_FIND_PACKAGE_NO_SYSTEM_PACKAGE_REGISTRY=ON -DCMAKE_INSTALL_SYSTEM_RUNTIME_LIBS_SKIP=TRUE -DCMAKE_VERBOSE_MAKEFILE=ON -DVCPKG_APPLOCAL_DEPS=OFF -DCMAKE_TOOLCHAIN_FILE=/Users/orudge/vcpkg/vcpkg/scripts/buildsystems/vcpkg.cmake -DCMAKE_ERROR_ON_ABSOLUTE_INSTALL_DESTINATION=ON -DVCPKG_CXX_FLAGS= -DVCPKG_CXX_FLAGS_RELEASE= -DVCPKG_CXX_FLAGS_DEBUG= -DVCPKG_C_FLAGS= -DVCPKG_C_FLAGS_RELEASE= -DVCPKG_C_FLAGS_DEBUG= -DVCPKG_CRT_LINKAGE=dynamic -DVCPKG_LINKER_FLAGS= -DVCPKG_TARGET_ARCHITECTURE=x64 -DCMAKE_INSTALL_LIBDIR:STRING=lib -DCMAKE_INSTALL_BINDIR:STRING=bin -G Ninja -DCMAKE_BUILD_TYPE=Debug -DCMAKE_INSTALL_PREFIX=/Users/orudge/vcpkg/vcpkg/packages/ignition-msgs5_x64-osx/debug
    Working Directory: /Users/orudge/vcpkg/vcpkg/buildtrees/ignition-msgs5/x64-osx-dbg
    Error code: 1
    See logs for more information:
      /Users/orudge/vcpkg/vcpkg/buildtrees/ignition-msgs5/config-x64-osx-dbg-out.log
      /Users/orudge/vcpkg/vcpkg/buildtrees/ignition-msgs5/config-x64-osx-dbg-err.log

Call Stack (most recent call first):
  scripts/cmake/vcpkg_configure_cmake.cmake:306 (vcpkg_execute_required_process)
  installed/x64-osx/share/ignitionmodularscripts/ignition_modular_library.cmake:3 (vcpkg_configure_cmake)
  installed/x64-osx/share/ignitionmodularscripts/ignition_modular_library.cmake:131 (ignition_modular_build_library)
  ports/ignition-msgs5/portfile.cmake:13 (ignition_modular_library)
  scripts/ports.cmake:90 (include)


Error: Building package ignition-msgs5:x64-osx failed with: BUILD_FAILED
Please ensure you're using the latest portfiles with `.\vcpkg update`, then
submit an issue at https://github.com/Microsoft/vcpkg/issues including:
  Package: ignition-msgs5:x64-osx
  Vcpkg version: 2020.02.04-unknownhash

It looks like the ignition-msgs5 port is looking for a non-existent vcpkg/packages/tinyxml2_x64-osx/debug/include folder - no other packages have a debug/include folder though, and I'm not quite sure where this is coming from for tinyxml2. Unfortunately I don't think this is something I have the time (or necessarily expertise) to debug further.

@traversaro
Copy link
Contributor

I think that at some point the CI machine has been modified to contain also the pkg-config command, and ignition-msgs5's by default uses pkg-config to find TinyXML2, and then uses a normal CMake-based search as a fallback. The tinyxml2 port has been installing faulty .pc files (see #10039), but as pkg-config was not installed in the CI machine, this problem was hidden. A possible workarond is either to fix #10039, or to modify the tinyxml2 port do not install .pc files until #10039 is fixed.

@orudge
Copy link
Contributor Author

orudge commented Jun 1, 2020

Thanks, that helps - the change I've pushed should fix #10039 too (via a patch).

@orudge orudge force-pushed the tinyxml2-fixed-exports branch from fd99a55 to 4dc579b Compare June 1, 2020 20:24
@NancyLi1013 NancyLi1013 added info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. and removed requires:author-response labels Jun 4, 2020
@NancyLi1013
Copy link
Contributor

Just need to rerun this PR once the regressions are fixed in this PR #11742.

@NancyLi1013 NancyLi1013 removed the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Jun 4, 2020
@NancyLi1013
Copy link
Contributor

@orudge
It seems that there are some new regressions generated. Could you please take a look?

@orudge orudge force-pushed the tinyxml2-fixed-exports branch from 4dc579b to 7e4a581 Compare June 8, 2020 10:40
@orudge
Copy link
Contributor Author

orudge commented Jun 8, 2020

It seems that there are some new regressions generated. Could you please take a look?

Most of these seem to be unrelated issues, primarily download errors or out of disk space (llvm). I can't find any issues relating to tinyxml2. I've rebased and pushed the changes again and it looks like the tests have failed again with a download error for a port.

@JackBoosY
Copy link
Contributor

/azp run

@JackBoosY JackBoosY requested a review from NancyLi1013 June 9, 2020 07:07
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@NancyLi1013 NancyLi1013 added info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. and removed requires:author-response labels Jun 10, 2020
@dan-shaw dan-shaw merged commit 50deb3e into microsoft:master Jun 12, 2020
@dan-shaw
Copy link
Contributor

Thanks for the PR!

JangBoo pushed a commit to JangBoo/vcpkg that referenced this pull request Jun 18, 2020
…tic libraries (microsoft#11616)

* [tinyxml2] Update to 8.0.0; avoid exporting symbols when building static libraries

* [tinyxml2] Clean up files
penumbra23 pushed a commit to codespace-dev/vcpkg that referenced this pull request Aug 5, 2020
…tic libraries (microsoft#11616)

* [tinyxml2] Update to 8.0.0; avoid exporting symbols when building static libraries

* [tinyxml2] Clean up files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:port-update The issue is with a library, which is requesting update new revision info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants