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

[lz4] depends on xxhash #28748

Merged
merged 1 commit into from
Jan 9, 2023
Merged

Conversation

autoantwort
Copy link
Contributor

Fixes #28747

github-actions[bot]
github-actions bot previously approved these changes Jan 5, 2023
github-actions[bot]
github-actions bot previously approved these changes Jan 5, 2023
github-actions[bot]
github-actions bot previously approved these changes Jan 5, 2023
@Neumann-A
Copy link
Contributor

You forgot to add the find_dependency call in the lz4-config.cmake.
You also might want to add a #pragma comment(lib, <xxhashlib>) to one of the main headers to simply autolink. Don't know if gcc has something similar.

@autoantwort
Copy link
Contributor Author

You forgot to add the find_dependency call in the lz4-config.cmake.

Can I also do that with

install(EXPORT lz4Config
  FILE lz4-config.cmake
  NAMESPACE lz4::
  DESTINATION "${INSTALL_CMAKE_DIR}"
)

@Neumann-A
Copy link
Contributor

Can I also do that with

only by reading that file from the portfile and adding the lines at the top.

@dg0yt
Copy link
Contributor

dg0yt commented Jan 5, 2023

You also might want to add a #pragma comment(lib, <xxhashlib>) to one of the main headers to simply autolink. Don't know if gcc has something similar.

This exists only in MSVC.

AFAICT lz4 would use the vendored xxhash with a specific name prefix (at least if not using vcpkg's vendored build system). As immediate fix, it would have been enough to not install this header. As another step, consider upstream's optional meson build.

@Neumann-A
Copy link
Contributor

As another step, consider upstream's optional meson build.

Probably easier and gets rid of the CMakeLists.txt vendored by vcpkg.

@FrankXie05 FrankXie05 added the category:port-bug The issue is with a library, which is something the port should already support label Jan 6, 2023
@dg0yt
Copy link
Contributor

dg0yt commented Jan 7, 2023

@vicroms We need this one merged ASAP. Baseline regression.

@FrankXie05
Copy link
Contributor

Temporarily merging it to fix issues with baseline.

@FrankXie05 FrankXie05 added the info:reviewed Pull Request changes follow basic guidelines label Jan 9, 2023
@vicroms vicroms merged commit 7d02e81 into microsoft:master Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[xxhash] file conflict with lz4
5 participants