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

[msdfgen] New port #15427

Merged
merged 12 commits into from Jan 17, 2021
Merged

[msdfgen] New port #15427

merged 12 commits into from Jan 17, 2021

Conversation

Haeri
Copy link
Contributor

@Haeri Haeri commented Jan 3, 2021

Describe the pull request

  • What does your PR fix? Fixes #
    New port for msdfgen

  • Which triplets are supported/not supported? Have you updated the CI baseline?
    Only static build

  • Does your PR follow the maintainer guide?
    Yes

@NancyLi1013 NancyLi1013 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Jan 4, 2021
ports/msdfgen/portfile.cmake Outdated Show resolved Hide resolved
ports/msdfgen/portfile.cmake Outdated Show resolved Hide resolved
ports/msdfgen/portfile.cmake Outdated Show resolved Hide resolved
@NancyLi1013
Copy link
Contributor

The failures on x64-uwp and arm-uwp:

CheckForUnsupportedScenarios:
         The specified XPath query did not capture any nodes.
     6>D:\buildtrees\msdfgen\x64-uwp-dbg\msdfgen-standalone.dir\package.appxManifest : error APPX0703: Manifest references file 'msdfgen-standalone.exe' which is not part of the payload. [D:\buildtrees\msdfgen\x64-uwp-dbg\msdfgen-standalone.vcxproj]
     6>Done Building Project "D:\buildtrees\msdfgen\x64-uwp-dbg\msdfgen-standalone.vcxproj" (default targets) -- FAILED.
     3>Done Building Project "D:\buildtrees\msdfgen\x64-uwp-dbg\ALL_BUILD.vcxproj" (default targets) -- FAILED.
     1>Done Building Project "D:\buildtrees\msdfgen\x64-uwp-dbg\install.vcxproj" (default targets) -- FAILED.

Could you please help look into this? @Haeri

If this port doesn't support uwp, please add supports: !uwp to vcpkg.json and add vcpkg_fail_port_install(ON_TARGET "uwp") to portfile.cmake.

Haeri and others added 3 commits January 4, 2021 05:14
Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>
Co-authored-by: NancyLi1013 <46708020+NancyLi1013@users.noreply.github.com>
@Haeri
Copy link
Contributor Author

Haeri commented Jan 4, 2021

The failures on x64-uwp and arm-uwp:

CheckForUnsupportedScenarios:
         The specified XPath query did not capture any nodes.
     6>D:\buildtrees\msdfgen\x64-uwp-dbg\msdfgen-standalone.dir\package.appxManifest : error APPX0703: Manifest references file 'msdfgen-standalone.exe' which is not part of the payload. [D:\buildtrees\msdfgen\x64-uwp-dbg\msdfgen-standalone.vcxproj]
     6>Done Building Project "D:\buildtrees\msdfgen\x64-uwp-dbg\msdfgen-standalone.vcxproj" (default targets) -- FAILED.
     3>Done Building Project "D:\buildtrees\msdfgen\x64-uwp-dbg\ALL_BUILD.vcxproj" (default targets) -- FAILED.
     1>Done Building Project "D:\buildtrees\msdfgen\x64-uwp-dbg\install.vcxproj" (default targets) -- FAILED.

Could you please help look into this? @Haeri

If this port doesn't support uwp, please add supports: !uwp to vcpkg.json and add vcpkg_fail_port_install(ON_TARGET "uwp") to portfile.cmake.

I have no idea what UWP is doing... Never dealt with it before. But from the error message it could be that it is expecting a msdfgen-standalone.exe since the solution has that name but the the binary actually being renamed to msdfgen.exe in the CMakeLists.txt:

add_executable(msdfgen-standalone main.cpp)
set_target_properties(msdfgen-standalone PROPERTIES ARCHIVE_OUTPUT_DIRECTORY archive OUTPUT_NAME msdfgen)

Not sure if this is maybe even a cmake bug?

@NancyLi1013
Copy link
Contributor

@Haeri Thanks for your further investigation.

I think there is no need to rename msdfgen-standalone as msdfgen. We can update as the follows:

Update set_target_properties(msdfgen-standalone PROPERTIES ARCHIVE_OUTPUT_DIRECTORY archive OUTPUT_NAME msdfgen)

as

set_target_properties(msdfgen-standalone PROPERTIES ARCHIVE_OUTPUT_DIRECTORY archive)

ports/msdfgen/portfile.cmake Outdated Show resolved Hide resolved
@Haeri
Copy link
Contributor Author

Haeri commented Jan 5, 2021

@Haeri Thanks for your further investigation.

I think there is no need to rename msdfgen-standalone as msdfgen. We can update as the follows:

Update set_target_properties(msdfgen-standalone PROPERTIES ARCHIVE_OUTPUT_DIRECTORY archive OUTPUT_NAME msdfgen)

as

set_target_properties(msdfgen-standalone PROPERTIES ARCHIVE_OUTPUT_DIRECTORY archive)

@NancyLi1013 I was hesitant to remove the renaming as the msdfgen.exe is referenced multiple time in the repository, namely in the bin/example.bat and README.md.

@NancyLi1013
Copy link
Contributor

@Haeri
Thanks for your clarification about this point.

Sorry for my fault. We should not change the property in this case. So just keep it as before.

For the failures on uwp, which is caused by the executable name, I also have no idea about this issue.

@strega-nil Could you please help take a look about this issue?

We noticed that there is still msdfgen-standalone.exe instead of msdfgen.exe in package.appxManifest on uwp platform.

F:\15427\vcpkg\buildtrees\msdfgen\x64-uwp-dbg\msdfgen-standalone.dir\package.appxManifest : error APPX0703: Manifest references file 'msdfgen-standalone.exe' which is not part of the payload. [F:\15427\vcpkg\buildtrees\msdfgen\x64-uwp-dbg\msdfgen-standalone.vcxproj]
     6>Done Building Project "F:\15427\vcpkg\buildtrees\msdfgen\x64-uwp-dbg\msdfgen-standalone.vcxproj" (default targets) -- FAILED.

@Haeri
Copy link
Contributor Author

Haeri commented Jan 6, 2021

#7202 Seems to have had the same issue, so maybe @JackBoosY knows more?

@NancyLi1013
Copy link
Contributor

Thanks again for your feedback. @Haeri

We might consider to drop the property of renaming this executable. Namely remove set_target_properties(msdfgen-standalone PROPERTIES ARCHIVE_OUTPUT_DIRECTORY archive OUTPUT_NAME msdfgen).

Besides this, I cannot figure out other solution to avoid this problem.

@Haeri
Copy link
Contributor Author

Haeri commented Jan 7, 2021

@NancyLi1013 I got around 90 code hits on github.com for msdfgen.exe so I think maybe it would be better to drop UWP instead. Or maybe there is a way to create a symlink from msdfgen-standalone.exe to msdfgen.exe so UWP doesn't complain about msdfgen-standalone.exe not existing and people relying on the existence of msdfgen.exe also being happy?

@Haeri
Copy link
Contributor Author

Haeri commented Jan 7, 2021

Interesting... this worked.. I was going to solve this with a symlink but seeing that they are very finicky especially on windows and the binary being only 200kb I think this could be a viable solution?

@JackBoosY
Copy link
Contributor

@Haeri I think that's okay, because the uwp tools can't be used directly. So please add it as a feature, make a patch to fix that.

@Haeri
Copy link
Contributor Author

Haeri commented Jan 8, 2021

@JackBoosY Sorry I did a lot of changes recently... not sure which one you are referring to... Also I don't know what you mean with add it as a feature?

@JackBoosY
Copy link
Contributor

JackBoosY commented Jan 8, 2021

@Haeri You can add a cmake option MSDFGEN_BUILD_TOOLS (option(MSDFGEN_BUILD_TOOLS "Build tools" OFF)) to its CMakeLists.txt, and add judgment if (MSDFGEN_BUILD_TOOLS) to add_executable(..).

@Haeri
Copy link
Contributor Author

Haeri commented Jan 8, 2021

@JackBoosY There is already an option natively given in the CMake. Its called MSDFGEN_BUILD_MSDFGEN_STANDALONE:
https://github.com/Chlumsky/msdfgen/blob/9af250c7d6780a41dcaf536c05e3e1987a1bdcd7/CMakeLists.txt#L102

@JackBoosY
Copy link
Contributor

@Haeri You shoud:

  1. Add feature tools in CONTROL file.
  2. In portfile.cmake:
vcpkg_from_github(...)

set(BUILD_TOOLS OFF)
if ("tools" IN_LIST FEATURES)
    if (VCPKG_TARGET_IS_UWP)
        message("Tools couldn't be built on UWP, disable it automatically.")
    else()
        set(BUILD_TOOLS ON)
    endif()
endif()
...
vcpkg_configure_cmake(
...
    options
        -DMSDFGEN_BUILD_MSDFGEN_STANDALONE=${BUILD_TOOLS}
)
...

@JackBoosY
Copy link
Contributor

@Haeri Ready for review now?

@Haeri Haeri marked this pull request as ready for review January 13, 2021 15:06
@NancyLi1013
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@NancyLi1013 NancyLi1013 added the info:reviewed Pull Request changes follow basic guidelines label Jan 15, 2021
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! info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants