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

[mxml] New Port #39991

Draft
wants to merge 33 commits into
base: master
Choose a base branch
from
Draft

[mxml] New Port #39991

wants to merge 33 commits into from

Conversation

Rossmaxx
Copy link
Contributor

@Rossmaxx Rossmaxx commented Jul 20, 2024

fixes: #19042

  • Changes comply with the maintainer guide.
  • The name of the port matches an existing name for this component on https://repology.org/ if possible, and/or is strongly associated with that component on search engines.
  • Optional dependencies are resolved in exactly one way. For example, if the component is built with CMake, all find_package calls are REQUIRED, are satisfied by vcpkg.json's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxx.
  • The versioning scheme in vcpkg.json matches what upstream says.
  • The license declaration in vcpkg.json matches what upstream says.
  • The installed as the "copyright" file matches what upstream says.
  • The source code of the component installed comes from an authoritative source.
  • The generated "usage text" is accurate. See adding-usage for context.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is in the new port's versions file.
  • Only one version is added to each modified port's versions file.

@Rossmaxx
Copy link
Contributor Author

Do note that this PR is incomplete. I don't really know what I'm doing here, so any docs and extra help and info appreciated. Most of what's written here is copied from other ports.

@Rossmaxx
Copy link
Contributor Author

What's with the license fail?

@FrankXie05
Copy link
Contributor

@Rossmaxx Thanks for this PR. I think we need to ask upstream whether to add it to vcpkg. In this way, I can help you complete this PR. :)

@FrankXie05 FrankXie05 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Jul 22, 2024
@Rossmaxx
Copy link
Contributor Author

Rossmaxx commented Jul 22, 2024

I think we need to ask upstream whether to add it to vcpkg

I did ask about this upstream some time ago, and here's what the maintainer said. Forgot that i asked this in the first place.

michaelrsweet/mxml#281 (comment)

I initially asked for cmake support but the maintainer refused. It's after some time that I understand that vcpkg doesn't require ports to be in cmake and other build systems being supported.

I personally need this port for a project which I'm involved with. I'm trying to add msvc support for a previously mingw built application.

For my use case, I am thinking of disabling linux ports as I don't really want to deal with pkg config here. I'll update the vckpg.json when i start working on this again. I'm on a small break now. But i would like to collect as much info on this break.

Thanks for your reply.

@dg0yt
Copy link
Contributor

dg0yt commented Oct 2, 2024

(I don't understand the comments about upstream. No patches, no restriction, no need to ask.)

@Rossmaxx
Copy link
Contributor Author

Rossmaxx commented Jan 2, 2025

Quite a lot has happened since the last time, Upstream has moved on to version 4, while this port is at version 3. For convenience sake, I'll be working on version 3, and the upstream suggests mxml4 to be a seperate package because of a breakage in compatibility.

@FrankXie05 @dg0yt sorry for the delay. Can you guys help me move this PR forward?

@dg0yt
Copy link
Contributor

dg0yt commented Jan 2, 2025

the upstream suggests mxml4 to be a seperate package because of a breakage in compatibility.

AFAICT vcpkg is reluctant to accept a "separate package". Usually it is not possible to install two variants of a package at the same time, but this is what vcpkg CI does.

@Rossmaxx
Copy link
Contributor Author

Rossmaxx commented Jan 2, 2025

For my use case, mxml 3 is fine. If need arises, I can make a seperate PR to bump the version.

@dg0yt
Copy link
Contributor

dg0yt commented Jan 2, 2025

For !windows, try at least with DETERMINE_BUILD_TRIPLET instead of AUTOCONFIG.

@Rossmaxx
Copy link
Contributor Author

Rossmaxx commented Jan 2, 2025

Everything mostly seems to work now, except for a few loose ends.

Building mxml[core]:x64-windows...
-- Using cached michaelrsweet-mxml-fd47c7d115191c8a6bce2c781ffee41e179530f2.tar.gz.
-- Cleaning sources at C:/vcpkg/buildtrees/mxml/src/1e179530f2-6bf12190f1.clean. Use --editable to skip cleaning for the packages you specify.
-- Extracting source C:/vcpkg/downloads/michaelrsweet-mxml-fd47c7d115191c8a6bce2c781ffee41e179530f2.tar.gz
-- Using source at C:/vcpkg/buildtrees/mxml/src/1e179530f2-6bf12190f1.clean
-- Getting CMake variables for x64-windows
-- Building vcnet/mxml1.vcxproj for Release
-- Building vcnet/mxml1.vcxproj for Debug
-- Installing: C:/vcpkg/packages/mxml_x64-windows/share/mxml/copyright
-- Performing post-build validation

# there's something wrong here
warning: The folder /include is empty or not present. This indicates the library was not correctly installed.
error: Found 1 post-build check problem(s). To submit these ports to curated catalogs, please first correct the portfile: C:\vcpkg\ports\mxml\portfile.cmake
Stored binary cache: 

"C:\Users\ROSHAN\AppData\Local\vcpkg\archives\98\98f5f42675b912e3f5b0e565833259e9b2ec105dce331332f03ec13c985639e2.zip"
Elapsed time to handle mxml:x64-windows: 22.42 s

can anyone help?

For !windows, try at least with DETERMINE_BUILD_TRIPLET instead of AUTOCONFIG.

can you explain?

@dg0yt
Copy link
Contributor

dg0yt commented Jan 2, 2025

vcpkg_configure_make was called with AUTOCONFIG. This is an extra step which failed. But it is optional. DETERMINE_BUILD_TRIPLET is enough to ensure proper configuration also for cross builds.

@Rossmaxx
Copy link
Contributor Author

Rossmaxx commented Jan 2, 2025

Didn't I restrict builds for windows?

@Rossmaxx
Copy link
Contributor Author

I need help with post build validation, @dg0yt @FrankXie05

@FrankXie05
Copy link
Contributor

FrankXie05 commented Jan 20, 2025

warning: The folder /include is empty or not present. This indicates the library was not correctly installed.
error: Found 1 post-build check problem(s). To submit these ports to curated catalogs, please first correct the portfile: C:\vcpkg\ports\mxml\portfile.cmake
Stored binary cache:

This port build does not install any header files available,
Add set(VCPKG_POLICY_EMPTY_INCLUDE_FOLDER enabled) to the top of portfile.cmake or install the header files manually through file (INSTALL )

@Rossmaxx
Copy link
Contributor Author

Rossmaxx commented Jan 26, 2025

I have upgraded this port to mxml v4, and it's better to leave v3 behind. I am also trying to add support for non-windows builds now, but the CI is failing, and the logs ain't really giving me much clues. I'll try troubleshooting from my linux partition later as i'm done with this for now. Did I make a mistake in the else part, or did I miss something?

I did fix the empty include folder issue tho, and I think it's coming along nicely.

vcpkg_configure_make was called with AUTOCONFIG. This is an extra step which failed. But it is optional. DETERMINE_BUILD_TRIPLET is enough to ensure proper configuration also for cross builds.

I remember seeing this before, but I forgot. Seeing this again now after putting the work into it.

I now also need to figure out a way to make this port work on cmake projects, if it doesn't by default.

@dg0yt more help please.

@dg0yt
Copy link
Contributor

dg0yt commented Jan 26, 2025

x64-linux CI:

make: *** No rule to make target 'mxml-attr.c', needed by 'mxml-attr.o'.  Stop.

Strange, but it might have an easy explanation. I could check later.

@Rossmaxx
Copy link
Contributor Author

Is this now an issue with the upstream's makefile?

@Rossmaxx
Copy link
Contributor Author

Rossmaxx commented Jan 26, 2025

here's the upstream, if it matters https://github.com/michaelrsweet/mxml/tree/v4.0.4

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.

For non-Windows, the key is DETERMINE_BUILD_TRIPLET and COPY_SOURCE.

Rossmaxx and others added 4 commits January 26, 2025 19:50
@Rossmaxx
Copy link
Contributor Author

I'm doing all this commits from my phone, I'll take some time to fix the versions.

Rossmaxx and others added 2 commits January 28, 2025 22:15
@Rossmaxx
Copy link
Contributor Author

What caused this build fail now? I'll investigate later.

@Rossmaxx
Copy link
Contributor Author

Rossmaxx commented Feb 1, 2025

Well, this is strange, it builds fine locally. What's with the CI fail then?

@dg0yt
Copy link
Contributor

dg0yt commented Feb 1, 2025

Locally, post-build checks are not fatal. For the official registry, they are.

-- Performing post-build validation
D:\a\_work\1\s\ports\mxml\portfile.cmake: warning: DLLs should not be present in a static build, but the following DLLs were found. To suppress this message, add set(VCPKG_POLICY_DLLS_IN_STATIC_LIBRARY enabled)
D:\p\mxml_x64-windows-static: note: the DLLs are relative to ${CURRENT_PACKAGES_DIR} here
note: debug/bin/mxml4.dll
note: bin/mxml4.dll
D:\a\_work\1\s\ports\mxml\portfile.cmake: warning: ${CURRENT_PACKAGES_DIR}/debug/bin exists but should not in a static build. To suppress this message, add set(VCPKG_POLICY_DLLS_IN_STATIC_LIBRARY enabled)
D:\a\_work\1\s\ports\mxml\portfile.cmake: warning: ${CURRENT_PACKAGES_DIR}/bin exists but should not in a static build. To suppress this message, add set(VCPKG_POLICY_DLLS_IN_STATIC_LIBRARY enabled)
note: if creation of these directories cannot be disabled, you can add the following in portfile.cmake to remove them
if(VCPKG_LIBRARY_LINKAGE STREQUAL "static")
  file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/debug/bin" "${CURRENT_PACKAGES_DIR}/bin")
endif()
D:\a\_work\1\s\ports\mxml\portfile.cmake: warning: Found 3 post-build check problem(s). These are usually caused by bugs in portfile.cmake or the upstream build system. Please correct these before submitting this port to the curated registry.
error: building mxml:x64-windows-static failed with: POST_BUILD_CHECKS_FAILED
See https://learn.microsoft.com/vcpkg/troubleshoot/build-failures?WT.mc_id=vcpkg_inproduct_cli for more information.

So basically, it always builds DLLs, but this unexpected for static triplets.

  • You may patch the msbuild project to create static libs.
  • You may make explicit the limitation with:
    if(VCPKG_TARGET_IS_WINDOWS)
        vcpkg_check_linkage(ONLY_DYNAMIC_LIBRARY)
    endif()
    but this implies "supports": "!(windows & static & staticcrt).
  • You may try to use the autotools build also for MSVC. But I have no idea if it works and helps for this project.

@Rossmaxx
Copy link
Contributor Author

Rossmaxx commented Feb 1, 2025

Ahh, forgot that static builds are a thing.

I should maybe patch in support for static builds as I'm pretty sure mxml is used as a static library in zyn. @JohannesLorenz is it so?

@JohannesLorenz
Copy link

I should maybe patch in support for static builds as I'm pretty sure mxml is used as a static library in zyn. @JohannesLorenz is it so?

@Rossmaxx When we compile, we just say link against it - it does not matter. For example, on my Linux, mxml is a shared library (sorry for the late reply!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[New Port Request] Mini-XML
4 participants