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

Fix cmake build (MSVC) #490

Merged
merged 2 commits into from
Dec 21, 2016
Merged

Fix cmake build (MSVC) #490

merged 2 commits into from
Dec 21, 2016

Conversation

jacquesg
Copy link
Contributor

No description provided.

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact [email protected] if you have any questions.

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@Cyan4973
Copy link
Contributor

cc @inikep , as I'm unsure if the proposed change is correct.
It seems to me it should better be libzstd.

I also note that we have currently no test to check this.
Maybe we need to add one in Appveyor.

@jacquesg
Copy link
Contributor Author

jacquesg commented Dec 19, 2016

This addresses a regression introduced in the 1.1.2 tag. There seems to be 2 rc files, build/VS2010/libzstd-dll/libzstd-dll.rc and build/VS2010/zstd/zstd.rc. The latter seems to be more up to date.

@inikep
Copy link
Contributor

inikep commented Dec 19, 2016

A good catch. The patch however is not good because cmake/lib/CMakeLists.txt generates a static and a dynamic library but zstd.rc is for zstd.exe as it contains e.g.
VALUE "OriginalFilename", "zstd.exe"
We should use build/VS2010/libzstd-dll/libzstd-dll.rc.

@jacquesg
Copy link
Contributor Author

Ok. Should I also bring the content of the 2 rc files in line with each other in terms of the LegalCopyright etc?

@inikep
Copy link
Contributor

inikep commented Dec 19, 2016

Thanks, but I already updated it in my branch.

@jacquesg
Copy link
Contributor Author

I've updated the PR. I've also added an extra commit which doesn't add the rc file to the static library.

@inikep
Copy link
Contributor

inikep commented Dec 19, 2016

You're right, the rc file is not required for the static library. But the line you've removed caused some errors detected with Travis CI:

CMake Error at lib/CMakeLists.txt:147 (SET_TARGET_PROPERTIES):
  set_target_properties Can not find target to add properties to:
  libzstd_static
the variable PREFIX=~/install_test_dir
CMake Error at lib/CMakeLists.txt:169 (INSTALL):
  install TARGETS given target "libzstd_static" which does not exist in this
  directory.

@inikep
Copy link
Contributor

inikep commented Dec 19, 2016

I think that
ADD_LIBRARY(libzstd_static STATIC ${Sources} ${Headers} ${PlatformDependResources})
should be replaced with
ADD_LIBRARY(libzstd_static STATIC ${Sources} ${Headers})

@jacquesg
Copy link
Contributor Author

Indeed, fixed.

@Cyan4973 Cyan4973 merged commit 0ffa03d into facebook:dev Dec 21, 2016
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.

4 participants