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

[ankurvdev-embedresource] adding new port #34401

Merged
merged 2 commits into from
Oct 19, 2023

Conversation

ankurvdev
Copy link
Contributor

@ankurvdev ankurvdev commented Oct 11, 2023

  • 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.

@dg0yt
Copy link
Contributor

dg0yt commented Oct 11, 2023

This needs a host tool when consomed. We must ensure that everything is always used from the vcpkg installation, not loaded via FetchContent. It might help to have a CI test port.
(BTW it is not the first resource compiler port.)

@ankurvdev
Copy link
Contributor Author

ankurvdev commented Oct 11, 2023

This needs a host tool when consomed. We must ensure that everything is always used from the vcpkg installation, not loaded via FetchContent. It might help to have a CI test port.

I've got vcpkg tests in the source repo already.CMakeLists.txt
which checks this for quite a few official and community triplets

Did you mean tests in the vcpkg repo ? Is there a pattern i can follow for creating a ci test port ?

(BTW it is not the first resource compiler port.)

All I could find was cmakerc.
Can you point me to a few others if they exist ?

@dg0yt
Copy link
Contributor

dg0yt commented Oct 11, 2023

I consider the resource compiler topic sensitive to cross-build issues. That's why I ask for test port, given the young project. Disclaimer: I'm only a community member.

Did you mean tests in the vcpkg repo ? Is there a pattern i can follow for creating a ci test port ?

This one was just merged:
https://github.com/microsoft/vcpkg/tree/master/scripts/test_ports/vcpkg-ci-soci
It is a simple portfile to configure and build a simple cmake project. This isn't only run in CI but can be used easily in manual tests with --overlay-ports=scripts/test-ports.
(Adding the test port immediately uncovered problems with dllexport declarations so creating it was immediately useful.)

I've got vcpkg tests in the source repo already.

The idea is to have a CMake project which is not tightly coupled with vcpkg (in the form of ci/sample/CMakeLists.txt), but uses the minimal CMake syntax without any assumptions about vcpkg, such as the soci test project. Basically the example from the README, decorated with a project header.
This would run in vcpkg CI for arm-neon-android (32 bit) on x64-linux (64 bit) host - is this covered in your CI?

(BTW it is not the first resource compiler port.)

All I could find was cmakerc. Can you point me to a few others if they exist ?

Maybe cmakerec is the only lean port, but there are big fishes which bring own tools, at least Qt. (The qt5 ports do not support cross builds although upstream does.)

@ankurvdev
Copy link
Contributor Author

Thanks for the pointers. Yeah I agree these sorts of cross-compile projects are extremely useful for any form of codegen tools

The CI project in source repo does cover cross-compilaton for the following triplets build

  • arm*-android
  • wasm32-emscripten (this one is especially tricky)
  • uwp
    (and mingw too)

Basically, The ci sample project is used to test both vcpkg-mode and add_subdirectory-mode (no cross-compile) which are listed on the README as integration choices.
(probably why it might seem a little less lean)
But when operating in vcpkg mode it relies on find_package which is the only thing needed.

That said, if you think it'll be prudent,
I can definitely follow the pattern you listed and wouldn't mind creating a leaner "vcpkg-only" test-port in the folder.

@ankurvdev
Copy link
Contributor Author

@dg0yt
I added a CI test and looks like I was mistaken and apparently find_program isn't working as I expected it.
My guess is I need to tweak these lines
but I'm not sure what the right pattern here is

Could you advise ?

@Cheney-W Cheney-W added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Oct 11, 2023
@ankurvdev ankurvdev marked this pull request as ready for review October 12, 2023 03:26
@dg0yt
Copy link
Contributor

dg0yt commented Oct 12, 2023

That find_program pattern is unusual, but the vcpkg toolchain wouldn't do the right thing anyways.

I would add a post-install fixup in the vcpkg portfile, based on the known vcpkg install layout. Something like

file(READ "${CURRENT_PACKAGES_DIR}/share/embedresource/EmbedResourceConfig.cmake" config)
file(WRITE "${CURRENT_PACKAGES_DIR}/share/embedresource/EmbedResourceConfig.cmake" "
find_program(EMBEDRESOURCE_EXECUTABLE embedresource PATHS "\${CMAKE_CURRENT_LIST_DIR}/../../../${HOST_TRIPLET}/tools/embedresource" REQUIRED)
${config}")

(Edit: removed backslash in last line.)

@ankurvdev
Copy link
Contributor Author

file(READ "${CURRENT_PACKAGES_DIR}/share/embedresource/EmbedResourceConfig.cmake" config)
file(WRITE "${CURRENT_PACKAGES_DIR}/share/embedresource/EmbedResourceConfig.cmake" "
find_program(EMBEDRESOURCE_EXECUTABLE embedresource PATHS "\${CMAKE_CURRENT_LIST_DIR}/../../../${HOST_TRIPLET}/tools/embedresource" REQUIRED)
\${config}")

That didnt quite work.

HOST_TRIPLET wasnt defined

@dg0yt
Copy link
Contributor

dg0yt commented Oct 12, 2023

That didnt quite work.

HOST_TRIPLET wasnt defined

Be careful about the variables and escaping. ${HOST_TRIPLET} is to be evaluated in the portfile where it is defined. Same for ${config} but I accidentally added a backslash 🤦. ${CMAKE_CURRENT_LIST_DIR} is really meant to be evaluated when it is is used, so this needs (and has) the backslash.

@ankurvdev
Copy link
Contributor Author

Thanks
Apparently a NO_DEFAULT_PATH was required as well for android to work correctly.

@Cheney-W Cheney-W marked this pull request as draft October 13, 2023 10:15
@ankurvdev ankurvdev marked this pull request as ready for review October 13, 2023 16:21
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.

I have another set of comments. It should be the last set from my side (community).

ports/embedresource/portfile.cmake Outdated Show resolved Hide resolved
ports/embedresource/portfile.cmake Outdated Show resolved Hide resolved
ports/embedresource/portfile.cmake Outdated Show resolved Hide resolved
Cheney-W
Cheney-W previously approved these changes Oct 18, 2023
@Cheney-W Cheney-W added the info:reviewed Pull Request changes follow basic guidelines label Oct 18, 2023
@JavierMatosD
Copy link
Contributor

@dg0yt, thank you for the review!

@ankurvdev, the port LGTM, but since this is a young project and it doesn't show up on repology, would you mind adding a prefix to the name?

@JavierMatosD JavierMatosD marked this pull request as draft October 18, 2023 17:50
@ankurvdev
Copy link
Contributor Author

Can you suggest a prefix ?

@JavierMatosD
Copy link
Contributor

Can you suggest a prefix ?

ankurvdev-embedresource?

@ankurvdev ankurvdev marked this pull request as ready for review October 18, 2023 23:02
@Cheney-W Cheney-W changed the title [embedresource] adding new port [ankurvdev-embedresource] adding new port Oct 19, 2023
@JavierMatosD JavierMatosD merged commit 85f2afa into microsoft:master Oct 19, 2023
15 checks passed
@ankurvdev ankurvdev deleted the pr/embedresource branch October 23, 2023 20:24
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