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

cmake: use configure_file() for creating the .pc file #2462

Merged
merged 1 commit into from
Jan 11, 2021

Conversation

lazka
Copy link
Contributor

@lazka lazka commented Jan 9, 2021

Escaping in add_custom_target() seems to depend on the shell used in the cmake
generator and using Ninja on Windows, which uses cmd.exe, results in stray backslashes
in the .pc file.

Instead of going through escaping hell just use configure_file() with the existing
libzstd.pc.in file already used by the simple Makefile based build system.

This fixes the .pc file syntax when building zstd with CMake+Ninja+gcc on Windows.

Escaping in add_custom_target() seems to depend on the shell used in the cmake
generator and using Ninja on Windows, which uses cmd.exe, results in stray backslashes
in the .pc file.

Instead of going through escaping hell just use configure_file() with the existing
libzstd.pc.in file already used by the simple Makefile based build system.

This fixes the .pc file syntax when building zstd with CMake+Ninja+gcc on Windows.
@lazka
Copy link
Contributor Author

lazka commented Jan 9, 2021

(re-using the libzstd.pc.in might lead to breakage if it's changed in the future without realizing that it's used in two places, so I can also copy it to the cmake dir if wanted)

@terrelln
Copy link
Contributor

Awesome! I don't know why the add_custom_target() indirection was used in the first place, but this is a great simplification!

This was causing me huge problems when I was trying to fix the escaping. I've added a test on Linux, which is passing on this PR, so looks like it is correctly escaped for Linux too.

@terrelln terrelln merged commit e7b7820 into facebook:dev Jan 11, 2021
@lazka
Copy link
Contributor Author

lazka commented Jan 11, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants