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.

Sign up for GitHub

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

Jump to bottom

[tinyxml2]: do not force export the symbols when building statically #27514

Merged
merged 3 commits into from
Oct 29, 2022
Merged

[tinyxml2]: do not force export the symbols when building statically #27514

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
55179ca
[tinyxml2]: do not force export the symbols when building statically
aminya Oct 28, 2022
c50746e
[tinyxml2]: check for TINYXML2_EXPORT on non windows
aminya Oct 28, 2022
13f5750
[tinyxml2] avoid redefinition of TINYXML2_LIB
aminya Oct 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions ports/tinyxml2/0001-fix-do-not-force-export-the-symbols-when-building-st.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
From a512d312db5604efe803a6fe088e7d582e04533f Mon Sep 17 00:00:00 2001
From: Amin Yahyaabadi <aminyahyaabadi74@gmail.com>
Date: Wed, 26 Oct 2022 18:21:29 -0700
Subject: [PATCH] fix: do not force export the symbols when building statically

---
CMakeLists.txt | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)

Comment on lines +1 to +9
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want to merge such headers. (Minimal diff.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These patches were autogenerated from the git commits.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't ask for reasons.
Just use git diff.

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 didn't ask for reasons.

I used the Git Patch VsCode extension. Not my first time making PRs with such patches here. If you have a particular patch style, consider enforcing it through CI or a friendly review.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wish there had been time for community review.
In 9 of 10 cases, reviewer would ask you to minimize the patches.

Copy link
Member

Choose a reason for hiding this comment

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

I'm going to poke other maintainers about this, that I tend to like these headers because it makes it easier figure out of upstream has merged them or not is showing. Will add the result to the maintainer guide.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @BillyONeal. As another example, see the two recent zlib patches.
I do prefer headers if they help to identify the patches as in the zlib cases.
OTOH the headers here do not add useful information.

Copy link
Member

Choose a reason for hiding this comment

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

@ras0219-msft @JavierMatosD @markle11m @vicroms @AugP @valeriaconde @dan-shaw, and I discussed this today.

@BillyONeal proposes:

  • If a commit header in a patch is exactly as submitted to upstream, which means it may help in determining if upstream has applied, keep that header.
  • Otherwise, do not include such a header.

@ras0219-msft proposes:

  • Never include any such headers.
  • When submitted to upstream, a comment should be added (in the PATCHES list) that points out how it was submitted and that is used instead.

@markle11m explicitly likes Robert's suggestion.

@vicroms notes that he is not opposed to comments in the patch files.

@BillyONeal to author a change adopting @ras0219-msft's suggestion to the maintainer guide.

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 8802fb8..87cda90 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -16,8 +16,10 @@ endif ()
## Main library build
##

-set(CMAKE_CXX_VISIBILITY_PRESET hidden)
-set(CMAKE_VISIBILITY_INLINES_HIDDEN YES)
+if (BUILD_SHARED_LIBS)
+ set(CMAKE_CXX_VISIBILITY_PRESET hidden)
+ set(CMAKE_VISIBILITY_INLINES_HIDDEN YES)
+endif()

add_library(tinyxml2 tinyxml2.cpp tinyxml2.h)
add_library(tinyxml2::tinyxml2 ALIAS tinyxml2)
@@ -36,11 +38,14 @@ target_compile_definitions(
set_target_properties(
tinyxml2
PROPERTIES
- DEFINE_SYMBOL "TINYXML2_EXPORT"
VERSION "${tinyxml2_VERSION}"
SOVERSION "${tinyxml2_VERSION_MAJOR}"
)

+if(BUILD_SHARED_LIBS)
+ target_compile_definitions(tinyxml2 PRIVATE "TINYXML2_EXPORT")
+endif()
+
if (tinyxml2_BUILD_TESTING)
add_executable(xmltest xmltest.cpp)
target_link_libraries(xmltest PRIVATE tinyxml2::tinyxml2)
--
2.37.3.windows.1

25 changes: 25 additions & 0 deletions ports/tinyxml2/0002-fix-check-for-TINYXML2_EXPORT-on-non-windows.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
From 4beabf9fe05a049577e3923327804851d6a05f24 Mon Sep 17 00:00:00 2001
From: Amin Yahyaabadi <aminyahyaabadi74@gmail.com>
Date: Thu, 27 Oct 2022 19:06:52 -0700
Subject: [PATCH] fix: check for TINYXML2_EXPORT on non windows

---
tinyxml2.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tinyxml2.h b/tinyxml2.h
index cfb1053..5a36901 100755
--- a/tinyxml2.h
+++ b/tinyxml2.h
@@ -72,7 +72,7 @@ distribution.
# else
# define TINYXML2_LIB
# endif
-#elif __GNUC__ >= 4
+#elif defined(TINYXML2_EXPORT) && __GNUC__ >= 4
# define TINYXML2_LIB __attribute__((visibility("default")))
#else
# define TINYXML2_LIB
--
2.37.3.windows.1

3 changes: 3 additions & 0 deletions ports/tinyxml2/portfile.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ vcpkg_from_github(
REF 9.0.0
SHA512 9C5CE8131984690DF302CA3E32314573B137180ED522C92FD631692979C942372A28F697FDB3D5E56BCF2D3DC596262B724D088153F3E1D721C9536F2A883367
HEAD_REF master
PATCHES
0001-fix-do-not-force-export-the-symbols-when-building-st.patch
0002-fix-check-for-TINYXML2_EXPORT-on-non-windows.patch
)

vcpkg_cmake_configure(
Expand Down
2 changes: 1 addition & 1 deletion ports/tinyxml2/vcpkg.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "tinyxml2",
"version-semver": "9.0.0",
"port-version": 1,
"port-version": 2,
"description": "A simple, small, efficient, C++ XML parser",
"homepage": "https://github.com/leethomason/tinyxml2",
"license": "Zlib",
Expand Down
2 changes: 1 addition & 1 deletion versions/baseline.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7438,7 +7438,7 @@
},
"tinyxml2": {
"baseline": "9.0.0",
"port-version": 1
"port-version": 2
},
"tl-expected": {
"baseline": "1.0.0",
Expand Down
5 changes: 5 additions & 0 deletions versions/t-/tinyxml2.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
{
"versions": [
{
"git-tree": "c88f30e7ca28b2de522a19a7c7b6d318664e3860",
"version-semver": "9.0.0",
"port-version": 2
},
{
"git-tree": "5ef0e856167ad66665f51776d78fcffb1fcc1cb1",
"version-semver": "9.0.0",
Expand Down