Skip to content

Conversation

@SunBlack
Copy link
Contributor

@SunBlack SunBlack commented Jan 27, 2026

  • Changes comply with the maintainer guide.
  • SHA512s are updated for each updated download.
  • The "supports" clause reflects platforms that may be fixed by this new version.
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

Notes:

  • The old cmake patch renamed this to copyright, but I think this makes no sense
  • The include patch was necessary as I had compiler errors within boost
  • I did not include the added includes from the old CMake patch, as they are not included in the header files and the maintainers themselves do not export them (perhaps deliberately?).
  • Using XercesC::XercesC instead of patching the MacOS (CoreFoundation) and Linux (ICU) dependencies manually

@SunBlack SunBlack force-pushed the libe57 branch 8 times, most recently from b297dda to 5b45f02 Compare January 28, 2026 02:28
@SunBlack SunBlack marked this pull request as ready for review January 28, 2026 02:38
Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

Cursory review.

-elseif(${CMAKE_SYSTEM_NAME} STREQUAL "Windows")
- add_definitions(-DWINDOWS)
-endif()
+find_package(XercesC REQUIRED)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
+find_package(XercesC REQUIRED)
+find_package(XercesC REQUIRED)
+set(XML_LIBRARIES XercesC::XercesC)

and get rid of the other changes for the XML_LIBRARIES.

In any case, this needs a find_dependency(XercesC) in the installed cmake config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried it now and added a small test port. But how I can run the test port locally? Didn't found a parameter for vcpkg ci to tell him, I don't want build the whole repo.

Copy link
Contributor

@dg0yt dg0yt Jan 29, 2026

Choose a reason for hiding this comment

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

Use --overlay-ports scripts/test_ports to make the test ports available regular vcpkg install.

Comment on lines 31 to 34
# There is no license file in the repo yet, except a link to http://libe57.org/license.html"
set(VCPKG_POLICY_SKIP_COPYRIGHT_CHECK enabled)
Copy link
Contributor

Choose a reason for hiding this comment

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

The reference to the license URL can be written to copyright.

Copy link
Member

@vicroms vicroms left a comment

Choose a reason for hiding this comment

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

Please apply dg0yt's suggestions

@vicroms vicroms marked this pull request as draft January 28, 2026 09:00
@SunBlack SunBlack force-pushed the libe57 branch 6 times, most recently from 2044f47 to 603e271 Compare January 29, 2026 01:41
ENDIF()
# FIXME: This probably should be set for both cases
- SET(E57RefImpl_LIBRARIES optimized ${E57RefImpl_LIBRARY_RELEASE} debug ${E57RefImpl_LIBRARY_DEBUG})
+ SET(E57RefImpl_LIBRARIES optimized ${E57RefImpl_LIBRARY_RELEASE} ${E57RefImpl_DEPENDENCIES} debug ${E57RefImpl_LIBRARY_DEBUG} ${E57RefImpl_DEPENDENCIES})
Copy link
Contributor

Choose a reason for hiding this comment

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

optimized/debug only work for the next item. So you just append once, literally at the end of the E57RefImpl_LIBRARIES block (which is a poor select_library_configurations):

list(APPEND E57RefImpl_LIBRARIES
    Boost::filesystem
    Boost::system
    XercesC::XercesC
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possible, this legacy syntax has long been removed from our projects, so I've slowly forgotten the details about it 😅

-set(XML_LIBRARIES ${Xerces_LIBRARY})
-set(XML_INCLUDE_DIRS ${Xerces_INCLUDE_DIR})
+set(XML_LIBRARIES XercesC::XercesC)
+set(XML_INCLUDE_DIRS)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick, CMake quirk: This command unsets XML_INCLUDE_DIRS in the current scope. To really set an empty value and to hide the parent scope:

Suggested change
+set(XML_INCLUDE_DIRS)
+set(XML_INCLUDE_DIRS "")

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.

3 participants