8000 [vcpkg-tool-swig] Add new tool port by cbentejac · Pull Request #36612 · microsoft/vcpkg · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[vcpkg-tool-swig] Add new tool port #36612

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

[vcpkg-tool-swig] Add new tool port #36612

wants to merge 2 commits into from
+62 −0

Conversation

cbentejac
Copy link
@cbentejac cbentejac commented Feb 6, 2024 •
edited
Loading
  • 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.

@JonLiu1993 JonLiu1993 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Feb 7, 2024
@cbentejac
Copy link
Author

@microsoft-github-policy-service agree

@JonLiu1993
Copy link
Member

@cbentejac, Thanks for your PR, looks the PR build failed in many triplets, this is error log, please take a look:

CMake Error at ports/swig/portfile.cmake:26 (file):
  file COPY cannot find "/mnt/vcpkg-ci/p/swig_x64-linux/bin/Lib": No such
  file or directory.
Call Stack (most recent call first):
  scripts/ports.cmake:170 (include)

m-kuhn
m-kuhn reviewed Mar 20, 2024
View reviewed changes
m-kuhn
m-kuhn reviewed Mar 20, 2024
View reviewed changes
@m-kuhn
Copy link
Contributor
m-kuhn commented Mar 20, 2024

It's a tool, should this be a release only build?

F.t.r. there's an alternative (similar): ports/swig/portfile.cmake

@cbentejac cbentejac force-pushed the add-swig-port branch 6 times, most recently from 2ba3329 to 5289061 Compare September 27, 2024 19:26
@cbentejac cbentejac marked this pull request as ready for review September 27, 2024 19:43
m-kuhn
m-kuhn reviewed Sep 28, 2024
View reviewed changes
dg0yt
dg0yt reviewed Oct 3, 2024
View reviewed changes
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 don't know if the vcpkg maintainers will accept a tool port ATM.
If yes, it might need a different name (vcpkg-tool-swig).

I would also encourage to add a tiny test project in scripts/test_ports. Given that swig seems to need resources which are moved around only for Windows, I wouldn't be surprised if this area needs more work.

@JonLiu1993
Copy link
Member

Ping @cbentejac for response.

@cbentejac cbentejac changed the title [swig] Add new port [vcpkg-tool-swig] Add new tool port Oct 11, 2024
@cbentejac
Copy link
Author

Thanks @dg0yt for your feedback. I renamed the port and applied your suggested changes. I haven't added a test project yet, though.

JonLiu1993
JonLiu1993 reviewed Oct 12, 2024
View reviewed changes
ports/vcpkg-tool-swig/portfile.cmake
Comment on lines +27 to +28
file(COPY "${CURRENT_PACKAGES_DIR}/bin/" DESTINATION "${CURRENT_PACKAGES_DIR}/tools/swig")
file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/bin")
Copy link
Member
@JonLiu1993 JonLiu1993 Oct 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cbentejac, Could you explain the purpose of these two lines? why put the lib folder under the tools folder?
image

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering it is supposed to be a tool, I thought it was better to have it being self-contained. Since vcpkg_copy_tools only moves the executable and DLLs, the first line ensures that the "Lib" folder is also copied in the final tools folder, and the second cleans everything up so we end up in a similar state as if we had added the AUTO_CLEAN in vcpkg_copy_tools options.

Should "Lib" be placed somewhere else?

Copy link
Contributor
@dg0yt dg0yt Oct 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you test by running swig -swiglib for the binary in installed. Lib should be placed where it is expected on Windows.

On non-windows, it might be useful to move the binary to manual-tools/vcpkg-tool-swig/swig, and install an sh script in tools/[name]/swig instead. This script would set SWIG_LIB relative to the script location, and then forward the args to the actual swig binary.

(Since this weekend, I do have a test case: itk[python] needs swig >= 4.2. And the portfile needs to process swiglib. Cf. #41525.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Since this weekend, I do have a test case: itk[python] needs swig >= 4.2. And the portfile needs to process swiglib. Cf. #41525.)

What about gdal[python]?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dg0yt Lib is correctly placed according to "swig -swiglib" on Windows. I'll work next week on implementing your suggestion for non-Windows platforms and use your test case. Thanks!

dg0yt
dg0yt reviewed Oct 12, 2024
View reviewed changes
ports/vcpkg-tool-swig/portfile.cmake
Comment on lines +22 to +27
vcpkg_copy_tools(
TOOL_NAMES swig
DESTINATION "${CURRENT_PACKAGES_DIR}/tools/swig"
)

file(COPY "${CURRENT_PACKAGES_DIR}/bin/" DESTINATION "${CURRENT_PACKAGES_DIR}/tools/swig")

This comment was marked as outdated.

Sign in to view

dg0yt
dg0yt reviewed Oct 12, 2024
View reviewed changes
ports/vcpkg-tool-swig/portfile.cmake
Comment on lines +22 to +28
vcpkg_copy_tools(
TOOL_NAMES swig
DESTINATION "${CURRENT_PACKAGES_DIR}/tools/swig"
)

file(COPY "${CURRENT_PACKAGES_DIR}/bin/" DESTINATION "${CURRENT_PACKAGES_DIR}/tools/swig")
file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/bin")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
vcpkg_copy_tools(
TOOL_NAMES swig
DESTINATION "${CURRENT_PACKAGES_DIR}/tools/swig"
)
file(COPY "${CURRENT_PACKAGES_DIR}/bin/" DESTINATION "${CURRENT_PACKAGES_D 6D40 IR}/tools/swig")
file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/bin")
vcpkg_copy_tools(TOOL_NAMES swig AUTO_CLEAN)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the Lib dir in the screenshot now. Maybe that's the explanation for manual copying/removal?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing on linux: swig -swiglib points to the packages dir. Is there a chance to make a relocatable build?

Copy link
Contributor
12C0

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, not yet. swig/swig#2509

@JonLiu1993
Copy link
Member

Pinging @cbentejac Can you address the review suggestions? Thanks!

@JonLiu1993
Copy link
Member

Note: I will be converting your PR to draft status. When you respond, please revert to "ready for review". That way, I can be aware that you've responded since you can't modify the tags.

@JonLiu1993 JonLiu1993 marked this pull request as draft November 15, 2024 07:35
@m-kuhn
Copy link
Contributor
m-kuhn commented Dec 19, 2024

@cbentejac do you have plans to continue working on this in the near future?

@cbentejac
Copy link
Author

Hi @m-kuhn,

Yes, I'm planning on finishing this port but haven't been able to find the time to wrap it up lately.

@LilyWangLL LilyWangLL self-assigned this Apr 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m-kuhn m-kuhn m-kuhn left review comments

@dg0yt dg0yt dg0yt left review comments

@JonLiu1993 JonLiu1993 JonLiu1993 left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@JonLiu1993 JonLiu1993

@LilyWangLL LilyWangLL

Labels
category:new-port The issue is requesting a new library to be added; consider making a PR!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cbentejac @JonLiu1993 @m-kuhn @dg0yt @LilyWangLL
0