[nethost] Update nethost #25004

lanyizi · 2022-05-30T11:31:02Z

Describe the pull request

What does your PR fix?

Addressed the issue with static library. The previous version was pushed long ago, back then __declspec(dllimport) was unconditionally enabled on the nethost.h when compiling Windows platform, which hampers its use as static library: https://github.com/dotnet/runtime/blob/188427d7e18102c45fc6d0e20c135e226f215992/src/installer/corehost/cli/nethost/nethost.h#L12
This PR updates the version of referenced .NET code base to .NET 6, which has a better support for static library: https://github.com/dotnet/runtime/blob/70ae3df4a6f3c92fb6b315afc405edd10ff38579/src/native/corehost/nethost/nethost.h#L15
This PR also adds imported targets, allowing users to find_package() and then target_link_library directly, without having to manually set up include directories or compiler definitions
Which triplets are supported/not supported? Have you updated the CI baseline?

Triplet and CI baseline should be same as before, although personally I couldn't test all platforms.
Does your PR follow the maintainer guide?

I guess yes
If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

Yes

If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/

github-actions

You have modified or added at least one portfile where deprecated functions are used.

If you feel able to do so, please consider migrating them to the new functions:
vcpkg_install_cmake -> vcpkg_cmake_install (from port vcpkg-cmake)
vcpkg_build_cmake -> vcpkg_cmake_build (from port vcpkg-cmake)
vcpkg_configure_cmake -> vcpkg_cmake_configure (Please remove the option PREFER_NINJA) (from port vcpkg-cmake)
vcpkg_fixup_cmake_targets -> vcpkg_cmake_config_fixup (from port vcpkg-cmake-config)

In the ports that use the new function, you have to add the corresponding dependencies:

{
  "name": "vcpkg-cmake",
  "host": true
},
{
  "name": "vcpkg-cmake-config",
  "host": true
}

The following files are affected:

ports/nethost/portfile.cmake

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

ports/nethost/vcpkg.json

Valid values for the license field can be found in the documentation

FrankXie05

Please note that these functions are about to be deprecated, please use the new method.
#25004 (review)

ports/nethost/portfile.cmake

ports/nethost/vcpkg.json

github-actions

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

ports/nethost/vcpkg.json

Valid values for the license field can be found in the documentation

lanyizi · 2022-05-31T06:08:13Z

I have updated the deprecated functions, for example replacing vcpkg_configure_cmake with vcpkg_cmake_configure, however now the build fails, saying "Unknown CMake command: vcpkg_cmake_configure"...
https://dev.azure.com/vcpkg/public/_build/results?buildId=72951&view=logs&j=878666d5-db33-5b27-9e7d-b0c7ee352005&t=3980505e-92f5-53ef-75a8-4f2e70b97516&l=6015

FrankXie05 · 2022-05-31T06:09:07Z

@lanyizi New functions need dependencies, add dependencies in vcpkg.json:

  "dependencies": [
    {
      "name": "vcpkg-cmake",
      "host": true
    },
    {
      "name": "vcpkg-cmake-config",
      "host": true
    }
  ],

ports/nethost/vcpkg.json

https://github.com/microsoft/vcpkg/blob/master/docs/maintainers/maintainer-guide.md#add-cmake-exports-in-an-unofficial--namespace

github-actions

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

ports/nethost/vcpkg.json

Valid values for the license field can be found in the documentation

github-actions

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

ports/nethost/vcpkg.json

Valid values for the license field can be found in the documentation

FrankXie05 · 2022-05-31T09:15:47Z

ports/nethost/usage

+    find_package(unofficial-nethost CONFIG REQUIRED)
+    target_link_libraries(main PRIVATE unofficial::nethost::nethost) # DLL
+    target_link_libraries(main PRIVATE unofficial::nethost::libnethost) # Static


find_package(unofficial-nethost CONFIG REQUIRED) target_link_libraries(main PRIVATE unofficial::nethost::nethost) # DLL target_link_libraries(main PRIVATE unofficial::nethost::libnethost) # Static

Is it necessary to separate the linking of dynamic and static libraries?

And this usage is not available.

CMake Error at /Users/vcpkg/Frank/vcpkg/scripts/buildsystems/vcpkg.cmake:573 (_add_executable): Target "test" links to target "unofficial::nethost::nethost" but the target was not found. Perhaps a find_package() call is missing for an IMPORTED target, or an ALIAS target is missing? Call Stack (most recent call first): CMakeLists.txt:5 (add_executable)

Example CMakeLists.txt:

1 cmake_minimum_required(VERSION 3.8) 2 3 project(test) 4 5 add_executable(test "test.cpp") 6 7 find_package(unofficial-nethost CONFIG REQUIRED) 8 target_link_libraries(test PRIVATE unofficial::nethost::nethost) # DLL 9 target_link_libraries(test PRIVATE unofficial::nethost::libnethost) # Static

unofficial::nethost::nethost was meant to be used for non-static triplet
So if the current triplet is something like x86-windows-static, then only unofficial::nethost::libnethost is available.

This is also because upstream decided to name the two targets differently, they build the same source twice (both as static and shared library) at the same time:
https://github.com/dotnet/runtime/blob/70ae3df4a6f3c92fb6b315afc405edd10ff38579/src/native/corehost/nethost/CMakeLists.txt#L22
https://github.com/dotnet/runtime/blob/70ae3df4a6f3c92fb6b315afc405edd10ff38579/src/native/corehost/nethost/CMakeLists.txt#L23

If this appears too weird I can edit the patch file so both target have same name. Should I do that?

FrankXie05 · 2022-05-31T09:36:41Z

I found that for upstream they don't want nethost to export both dynamic and static libraries. https://github.com/dotnet/runtime/blob/55e2378d86841ec766ee21d5e504d7724c39b53b/src/native/corehost/nethost/nethost.h#L13.
There is nothing wrong with using the macro NETHOST_USE_AS_STATIC to differentiate, but can we add an option
like -DBUILD_SHARED or -DBUILD_STATIC to differentiate the usage of the exported library?(Maybe this way of yours is better, I'm not sure. I am testing your usage) 🤔

dg0yt · 2022-05-31T10:13:51Z

can we add an option
like -DBUILD_SHARED or -DBUILD_STATIC to differentiate the usage of the exported library?

Don't invent a new option. Just leverage BUILD_SHARED_LIBS.
https://cmake.org/cmake/help/latest/variable/BUILD_SHARED_LIBS.html

lanyizi · 2022-05-31T10:41:02Z

Don't invent a new option. Just leverage BUILD_SHARED_LIBS. https://cmake.org/cmake/help/latest/variable/BUILD_SHARED_LIBS.html

I feel BUILD_SHARED_LIBS won't have any effect when building the nethost, because upstream explicitly defined shared / dynamic targets which are being built at the same time...
https://github.com/dotnet/runtime/blob/v6.0.5/src/native/corehost/lib.cmake
https://github.com/dotnet/runtime/blob/v6.0.5/src/native/corehost/lib_static.cmake

dg0yt · 2022-05-31T10:58:48Z

I feel BUILD_SHARED_LIBS won't have any effect when building the nethost, because upstream explicitly defined shared / dynamic targets which are being built at the same time...

Yes, this CMake anti-pattern is known from other ports. But vcpkg wants only one variant per triplet.

You can add patches which use BUILD_SHARED_LIBS for if/else in the CMake build system.
BUILD_SHARED_LIBS is already passed by vcpkg as needed by the triplet.

You can give a usage example which makes use of generator expressions to select the library name.
Example: Port zstd,

vcpkg/ports/zstd/usage

Lines 1 to 4 in d953973

    
           The package zstd provides CMake targets: 
        
               find_package(zstd CONFIG REQUIRED) 
        
               target_link_libraries(main PRIVATE $<IF:$<TARGET_EXISTS:zstd::libzstd_shared>,zstd::libzstd_shared,zstd::libzstd_static>)

lanyizi · 2022-05-31T11:40:14Z

You can give a usage example which makes use of generator expressions to select the library name.
Example: Port zstd,

vcpkg/ports/zstd/usage

Lines 1 to 4 in d953973

The package zstd provides CMake targets:

find_package(zstd CONFIG REQUIRED)

target_link_libraries(main PRIVATE $<IF:$<TARGET_EXISTS:zstd::libzstd_shared>,zstd::libzstd_shared,zstd::libzstd_static>)

I feel this is easier to do for me (

github-actions

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

ports/nethost/vcpkg.json

Valid values for the license field can be found in the documentation

JackBoosY · 2022-06-01T09:25:55Z

Never mind the requests, did you test the usage locally?

lanyizi · 2022-06-01T09:28:05Z

Yes, I tested it with both x86-windows and x86-windows-static triplet
If needed I think I could also test x64-windows, but personally I can't test on other platforms (arm or linux)

ras0219-msft

Thanks for the PR!

I have reworked the patch to do a few additional things:

Always install the name "unofficial::nethost::nethost", to avoid users needing to write an awful generator expression. If a static build is requested, it's just a link to the libnethost target
Embed the library linkage choice into the header
I searched the source and found no uses of CLI_CMAKE_PLATFORM_ARCH, but I did see CLR_CMAKE_TARGET_ARCH, so I renamed that value.

Please run the following to merge those commits into this PR:

git fetch https://github.com/ras0219-msft/vcpkg dev/roschuma/nethost
git merge FETCH_HEAD

github-actions

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

ports/nethost/vcpkg.json

Valid values for the license field can be found in the documentation

lanyizi · 2022-06-01T21:16:50Z

Thanks for the rework! I have merged the new commit into PR

github-actions

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

ports/nethost/vcpkg.json

Valid values for the license field can be found in the documentation

lanyizi · 2022-06-01T22:44:40Z

@ras0219-msft Hi! I noticed there was some build errors, for example now the static build didn't include(../lib.cmake) anymore, however lib.cmake introduced some source files which are needed by the static build.
I edited the patch file so those source files are added to static target too.

github-actions

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

ports/nethost/vcpkg.json

Valid values for the license field can be found in the documentation

github-actions

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

ports/nethost/vcpkg.json

Valid values for the license field can be found in the documentation

ras0219-msft

Thanks!

vcpkg 那边的依赖可能要等下面这个 PR 通过之后才能使用： microsoft/vcpkg#25004 或者可以暂时把 vcpkg 的 git 分支切换到我的 fork，然后就能直接安装依赖了（

lanyizi added 2 commits May 30, 2022 13:19

update nethost

9242f41

bump nethost version

16f8683

github-actions bot approved these changes May 30, 2022

View reviewed changes

lanyizi changed the title ~~Update nethost~~ [nethost] Update nethost May 30, 2022

FrankXie05 self-assigned this May 31, 2022

FrankXie05 added the category:port-update The issue is with a library, which is requesting update new revision label May 31, 2022

FrankXie05 requested changes May 31, 2022

View reviewed changes

github-actions bot approved these changes May 31, 2022

View reviewed changes

FrankXie05 requested changes May 31, 2022

View reviewed changes

ports/nethost/vcpkg.json Outdated Show resolved Hide resolved

FrankXie05 added the requires:author-response label May 31, 2022

FrankXie05 requested changes May 31, 2022

View reviewed changes

ports/nethost/vcpkg.json Outdated Show resolved Hide resolved

lanyizi added 2 commits May 31, 2022 09:29

Add CMake exports in an unofficial- namespace

8082b31

https://github.com/microsoft/vcpkg/blob/master/docs/maintainers/maintainer-guide.md#add-cmake-exports-in-an-unofficial--namespace

fix port version and deprecated functions

8984bc1

lanyizi force-pushed the update-nethost branch from 51f2670 to 2e71ace Compare May 31, 2022 08:36

github-actions bot approved these changes May 31, 2022

View reviewed changes

change nethost version date to version

62d20f9

lanyizi force-pushed the update-nethost branch from 2e71ace to 50d39bd Compare May 31, 2022 08:39

github-actions bot approved these changes May 31, 2022

View reviewed changes

lanyizi requested a review from FrankXie05 May 31, 2022 09:13

FrankXie05 requested changes May 31, 2022

View reviewed changes

FrankXie05 linked an issue May 31, 2022 that may be closed by this pull request

[nethost] update to 2020-10-19 #14867

Closed

update usage

4ad2278

lanyizi force-pushed the update-nethost branch from 50d39bd to 7dc0ace Compare May 31, 2022 11:43

JackBoosY added the requires:author-response label Jun 1, 2022

lanyizi force-pushed the update-nethost branch from 7dc0ace to f99d9e2 Compare June 1, 2022 08:42

github-actions bot approved these changes Jun 1, 2022

View reviewed changes

lanyizi added 2 commits June 1, 2022 11:35

add quotes to paths and export targets directly

bc3f614

bump nethost version

9c64735

lanyizi force-pushed the update-nethost branch from f99d9e2 to 9c64735 Compare June 1, 2022 09:37

JackBoosY approved these changes Jun 1, 2022

View reviewed changes

JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Jun 1, 2022

[nethost] Only build one flavor

45fe29b

ras0219-msft requested changes Jun 1, 2022

View reviewed changes

ras0219-msft added the requires:author-response label Jun 1, 2022

github-actions bot approved these changes Jun 1, 2022

View reviewed changes

lanyizi requested a review from ras0219-msft June 1, 2022 22:44

lanyizi force-pushed the update-nethost branch from 4e87f1d to d4782f6 Compare June 1, 2022 22:48

github-actions bot approved these changes Jun 1, 2022

View reviewed changes

lanyizi added 2 commits June 2, 2022 00:49

fix build error

8a57237

bump nethost version

b100919

lanyizi force-pushed the update-nethost branch from d4782f6 to b100919 Compare June 1, 2022 22:50

github-actions bot approved these changes Jun 1, 2022

View reviewed changes

JackBoosY removed the requires:author-response label Jun 2, 2022

ras0219-msft approved these changes Jun 2, 2022

View reviewed changes

ras0219-msft merged commit 2aed520 into microsoft:master Jun 2, 2022

lanyizi deleted the update-nethost branch June 18, 2022 15:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[nethost] Update nethost #25004

[nethost] Update nethost #25004

lanyizi commented May 30, 2022

github-actions bot left a comment

FrankXie05 left a comment

github-actions bot left a comment

lanyizi commented May 31, 2022

FrankXie05 commented May 31, 2022

github-actions bot left a comment

github-actions bot left a comment

FrankXie05 May 31, 2022

FrankXie05 May 31, 2022

lanyizi May 31, 2022

FrankXie05 commented May 31, 2022

dg0yt commented May 31, 2022

lanyizi commented May 31, 2022

dg0yt commented May 31, 2022

lanyizi commented May 31, 2022

github-actions bot left a comment

JackBoosY commented Jun 1, 2022

lanyizi commented Jun 1, 2022

ras0219-msft left a comment

github-actions bot left a comment

lanyizi commented Jun 1, 2022

github-actions bot left a comment

lanyizi commented Jun 1, 2022 •

edited

Loading

github-actions bot left a comment

github-actions bot left a comment

ras0219-msft left a comment

[nethost] Update nethost #25004

[nethost] Update nethost #25004

Conversation

lanyizi commented May 30, 2022

What does your PR fix?

Which triplets are supported/not supported? Have you updated the CI baseline?

Does your PR follow the maintainer guide?

If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

github-actions bot left a comment

Choose a reason for hiding this comment

FrankXie05 left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

lanyizi commented May 31, 2022

FrankXie05 commented May 31, 2022

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

FrankXie05 May 31, 2022

Choose a reason for hiding this comment

FrankXie05 May 31, 2022

Choose a reason for hiding this comment

lanyizi May 31, 2022

Choose a reason for hiding this comment

FrankXie05 commented May 31, 2022

dg0yt commented May 31, 2022

lanyizi commented May 31, 2022

dg0yt commented May 31, 2022

lanyizi commented May 31, 2022

github-actions bot left a comment

Choose a reason for hiding this comment

JackBoosY commented Jun 1, 2022

lanyizi commented Jun 1, 2022

ras0219-msft left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

lanyizi commented Jun 1, 2022

github-actions bot left a comment

Choose a reason for hiding this comment

lanyizi commented Jun 1, 2022 • edited Loading

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

ras0219-msft left a comment

Choose a reason for hiding this comment

If you have added/updated a port: Have you run `./vcpkg x-add-version --all` and committed the result?

lanyizi commented Jun 1, 2022 •

edited

Loading