[yasm/yasm-tool] Incorporate yasm-tool into yasm by JackBoosY · Pull Request #18816 · microsoft/vcpkg

JackBoosY · 2021-07-05T08:00:49Z

Since port yasm and yasm-tool repo is same and generate the same files, remove port yasm-tool and switch the related dependency item yasm-tool to yasm.
Add option DYNAMIC_DEPENDS to vcpkg_copy_tool and vcpkg_copy_tool_dependencies to copy the dynamic dependencies (Dynamically load dependencies in code) too tools/${PORT}.

Fixes #18746.

…PENS

JackBoosY · 2021-07-05T08:31:53Z

cc @BillyONeal

JackBoosY · 2021-07-05T09:32:11Z

Note that if the DYNAMIC_DEPENDS change is approved, all ports that call vcpkg_copy_tool_dependencies should be modified.
(Already done that, didn't push to this branch yet)

scripts/cmake/vcpkg_copy_tool_dependencies.cmake

scripts/cmake/vcpkg_copy_tools.cmake

docs/maintainers/vcpkg_copy_tool_dependencies.md

cenit · 2021-07-06T14:28:49Z

note also the DEPENS typo (should be corrected into DEPENDS - or even better DEPENDENCIES?)

BillyONeal

I agree with @dg0yt 's feedback

docs/maintainers/vcpkg_copy_tool_dependencies.md

JackBoosY · 2021-12-14T09:50:36Z

Successfully tested the following affected ports:

mpg123 requires yasm_tool_helper
alac requires vcpkg_copy_tool_dependencies
cpuinfo requires vcpkg_copy_tools

JackBoosY · 2021-12-14T09:53:48Z

Ping @dg0yt @ras0219-msft @BillyONeal for review again.

ports/yasm/vcpkg-port-config.cmake.in

BillyONeal

I think this is a good change in concept; I didn't realize that we already had a yasm port that produced the yasm binaries when I added yasm-tool and friends, and smashing that all together is great. However, I don't understand the value we gain by adding things to a shared helper over it, particularly in a way that mixes positional and named parameters.

Can the yasm-tool port just contain the right renames itself?
If that is unreasonable for some reason or we expect this to be common, can we add a new function that copies these dynamic DLL dependencies rather than piggybacking on vcpkg_copy_tool_dependencies and its associated applocal.ps1 baggage?

BillyONeal · 2021-12-22T04:34:11Z

scripts/cmake/vcpkg_copy_tool_dependencies.cmake

-vcpkg_copy_tool_dependencies(<${CURRENT_PACKAGES_DIR}/tools/${PORT}>)
+vcpkg_copy_tool_dependencies(
+    <${CURRENT_PACKAGES_DIR}/tools/${PORT}>
+    [DEPENDENCIES <dep1>...]


This documentation suggests that the function uses named arguments but the implementation appears to use positional arguments.

Why can't the yasm port issue the right file(COPY calls itself rather than modifying a shared helper like this?

The yasm example shows that there are still other ports that use the same method (load_library) to depend on other libraries, which cann't be analysis by dumpbin.
So this is a common change.

I mean, there are ports that do that, yes, but I'm not seeing what's made clearer about this vs. just issuing the file(COPYs, and doing it like this creates the problems I listed below about mixing positional and named parameters.

BillyONeal · 2021-12-22T04:35:21Z

scripts/cmake/vcpkg_copy_tool_dependencies.cmake

 function(vcpkg_copy_tool_dependencies tool_dir)
-    if(ARGC GREATER 1)
+    cmake_parse_arguments(PARSE_ARGV 0 arg "" "" "DEPENDENCIES")
+    if(ARGC GREATER 2)


ARGC is going to be greater than 2 if tool_dir and DEPENDENCIES are both used; I don't think we should be mixing up positional and named parameters.

tool_dir should only be passed one value. what is your opinion?

I don't understand this response. tool_dir is only passed one value, but given

vcpkg_copy_tool_dependencies("C:/this/is/tool_dir" DEPENDENCIES "foo.dll")

ARGC is 3 so we're going to print that warning, which seems unintended.

These can't be correct

BillyONeal · 2021-12-22T04:36:01Z

ports/yasm-tool/portfile.cmake

-
-configure_file("${CMAKE_CURRENT_LIST_DIR}/vcpkg-port-config.cmake.in"
-    "${CURRENT_PACKAGES_DIR}/share/${PORT}/vcpkg-port-config.cmake" @ONLY)
+SET(VCPKG_POLICY_EMPTY_PACKAGE enabled)


Should this print a warning that it's now defunct?

According to Robert's comment, we should not remove any ports.
So I think that's okay, both yasm-tool or yasm[tools] can be used.

We can't remove it because that would break people's scripts and stuff, but we can warn that they aren't using the canonical one.

JackBoosY · 2022-02-22T06:42:07Z

In favor of #23218.

[yasm/yasm-tool] Remove port yasm-tool

c35a661

JackBoosY added category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist category:port-bug The issue is with a library, which is something the port should already support info:internal labels Jul 5, 2021

JackBoosY assigned PhoebeHui Jul 5, 2021

JackBoosY added 4 commits July 5, 2021 01:01

[vcpkg_copy_tools/vcpkg_copy_tool_dependencies] Add option DYNAMIC_DE…

105a82a

…PENS

[gmp] Switch dependency yasm-tool to yasm

c8ab16d

Update docs

40497a6

update docs

3eda1b7

PhoebeHui added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Jul 5, 2021

Add keyword TOOL_DIR to all used ports

75f540e

JackBoosY requested a review from BillyONeal July 5, 2021 09:32

JackBoosY added info:world-rebuild category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly labels Jul 5, 2021

JackBoosY marked this pull request as ready for review July 5, 2021 09:51

dg0yt reviewed Jul 5, 2021

View reviewed changes

BillyONeal reviewed Jul 6, 2021

View reviewed changes

JackBoosY added 4 commits July 8, 2021 19:37

Apply suggestion

ef230d1

fix typo

f6f3b99

typo

2a122d9

Update doc

0c70aa2

dg0yt reviewed Jul 9, 2021

View reviewed changes

docs/maintainers/vcpkg_copy_tool_dependencies.md Outdated Show resolved Hide resolved

JackBoosY added 4 commits July 8, 2021 23:32

Fix path

7e5d4fe

set TOOL_DIR default value

10bda64

update doc

d8b2e83

Correct the tool path

edcef24

BillyONeal approved these changes Jul 12, 2021

View reviewed changes

github-actions bot approved these changes Dec 14, 2021

View reviewed changes

JackBoosY added 2 commits December 14, 2021 01:38

Correct code

14d35f5

version

a6c385b

github-actions bot approved these changes Dec 14, 2021

View reviewed changes

JackBoosY changed the title ~~[yasm/yasm-tool] Remove port yasm-tool~~ [yasm/yasm-tool] Incorporate yasm-tool into yasm Dec 14, 2021

JackBoosY marked this pull request as ready for review December 14, 2021 09:52

JackBoosY added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Dec 14, 2021

JackBoosY commented Dec 14, 2021

View reviewed changes

ports/yasm/vcpkg-port-config.cmake.in Outdated Show resolved Hide resolved

JackBoosY added 4 commits December 14, 2021 18:39

[gmp] Change dependency yasm-tool to yasm

c5affe2

Update doc

0021966

Change to host path instead

94cfd28

version

b209da2

github-actions bot approved these changes Dec 15, 2021

View reviewed changes

JackBoosY requested a review from PhoebeHui December 17, 2021 02:38

PhoebeHui requested a review from ras0219-msft December 17, 2021 09:14

PhoebeHui approved these changes Dec 20, 2021

View reviewed changes

PhoebeHui added the info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. label Dec 20, 2021

BillyONeal requested changes Dec 22, 2021

View reviewed changes

BillyONeal added requires:author-response and removed info:needs-maintainer-attention Lets the current 'on rotation' vcpkg maintainer know they need to look at this. labels Dec 22, 2021

JackBoosY removed the requires:author-response label Dec 23, 2021

JackBoosY requested a review from BillyONeal December 23, 2021 13:57

PhoebeHui marked this pull request as draft January 30, 2022 02:10

JackBoosY removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Jan 30, 2022

JackBoosY mentioned this pull request Feb 22, 2022

[yasm/yasm-tool] Incorporate yasm-tool into yasm #23218

Merged

JackBoosY closed this Feb 22, 2022

JackBoosY deleted the dev/jack/18746 branch February 22, 2022 06:46

Conversation

JackBoosY commented Jul 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JackBoosY commented Jul 5, 2021

Uh oh!

JackBoosY commented Jul 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cenit commented Jul 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BillyONeal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JackBoosY commented Dec 14, 2021

Uh oh!

JackBoosY commented Dec 14, 2021

Uh oh!

Uh oh!

BillyONeal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JackBoosY commented Feb 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

JackBoosY commented Jul 5, 2021 •

edited

Loading

JackBoosY commented Jul 5, 2021 •

edited

Loading

cenit commented Jul 6, 2021 •

edited

Loading