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

[hexl] Add new port #18043

Merged
merged 1 commit into from
Jun 9, 2021
Merged

[hexl] Add new port #18043

merged 1 commit into from
Jun 9, 2021

Conversation

WeiDaiWD
Copy link
Contributor

Describe the pull request

  • What does your PR fix?

    Added a new port "hexl".

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

x64 & (windows | linux | osx) & static
No

Yes

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

I ran ./vcpkg x-add-version hexl for this new port only.

@WeiDaiWD
Copy link
Contributor Author

I'm working on fixing this, in multiple steps:

  • hexl downloads a dependency cpu-features. I can add cpu-features as a feature/dependency to hexl by adding a patch to ports/hexl.
  • The cpu-features port is wrong. The CpuFeaturesTargets.cmake exports an executable target and checks whether this executable file exists. The file does not exist since vcpkg probably have deleted all executables. Therefore, if cpu-features is installed via vcpkg, find_package(CpuFeatures) will always fail. To fix this, I also need to add a patch to ports/cpu-features.

@WeiDaiWD
Copy link
Contributor Author

I do need help for one thing. It seems only x64_windows_static and x64_windows_static_md are tested, despite that in ports/hexl/CONTROL I put Supports: x64 & (linux | osx | windows) & static. The correct list of supported triplets are: x64_windows_static, x64_windows_static_md, x64_linux, and x64_osx. What did I miss here?

Much appreciated!

@PhoebeHui PhoebeHui added the category:new-port The issue is requesting a new library to be added; consider making a PR! label May 21, 2021
@NancyLi1013 NancyLi1013 changed the title Added a new port hexl. [hexl] Add new port May 21, 2021
Copy link
Contributor

@NancyLi1013 NancyLi1013 left a comment

Choose a reason for hiding this comment

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

If we use new functions, vcpkg-cmake and vcpkg-cmake-config should be added to the dependencies.

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

ports/hexl/CONTROL Outdated Show resolved Hide resolved
ports/hexl/portfile.cmake Outdated Show resolved Hide resolved
ports/hexl/portfile.cmake Outdated Show resolved Hide resolved
ports/hexl/portfile.cmake Outdated Show resolved Hide resolved
ports/hexl/portfile.cmake Outdated Show resolved Hide resolved
ports/hexl/portfile.cmake Outdated Show resolved Hide resolved
ports/hexl/portfile.cmake Outdated Show resolved Hide resolved
@NancyLi1013
Copy link
Contributor

Hi @WeiDaiWD
Thanks for adding this new port, I noticed that upstream seems also support dynamic, why do you only support static build in this PR? Can you help make a simple description?

Thanks.

@WeiDaiWD
Copy link
Contributor Author

I've created a separate PR #18101 to fix port cpu-features. I'll continue on this one once that works.

@NancyLi1013 NancyLi1013 added the depends:different-pr This PR or Issue depends on a PR which has been filed label May 25, 2021
@WeiDaiWD WeiDaiWD force-pushed the master branch 4 times, most recently from 2b42eae to 950b29e Compare May 26, 2021 04:36
@NancyLi1013 NancyLi1013 removed the depends:different-pr This PR or Issue depends on a PR which has been filed label Jun 2, 2021
@NancyLi1013
Copy link
Contributor

This PR should be still in progress. I reopen it. You can merge master to rebuild since PR #18101 has been merged. @WeiDaiWD

@WeiDaiWD
Copy link
Contributor Author

WeiDaiWD commented Jun 7, 2021

x64-windows fails because the hexl library does not export its symbols. I added an extra condition in supports in the manifest file.

ports/hexl/disable_downloading_cpu_features.patch Outdated Show resolved Hide resolved
ports/hexl/disable_downloading_cpu_features.patch Outdated Show resolved Hide resolved
ports/hexl/disable_downloading_cpu_features.patch Outdated Show resolved Hide resolved
ports/hexl/disable_downloading_cpu_features.patch Outdated Show resolved Hide resolved
ports/hexl/disable_downloading_cpu_features.patch Outdated Show resolved Hide resolved
@WeiDaiWD
Copy link
Contributor Author

WeiDaiWD commented Jun 8, 2021

Will do. I've also just noticed that I have to make more changes to the patch, since hexl updated the commit hash for tag 1.1.0.

@NancyLi1013 NancyLi1013 added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Jun 9, 2021
@NancyLi1013
Copy link
Contributor

LGTM now, thanks for adding this new port. @WeiDaiWD

@strega-nil-ms
Copy link
Contributor

LGTM, thanks @WeiDaiWD :)

@strega-nil-ms strega-nil-ms merged commit c423e49 into microsoft:master Jun 9, 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.

4 participants