Cmake bug fix: CMake does not install zstdgrep and zstdless#1625
Cmake bug fix: CMake does not install zstdgrep and zstdless#1625LeeYoung624 wants to merge 5 commits intofacebook:devfrom LeeYoung624:cmake_install_fix
Conversation
Mno-hime
left a comment
Mno-hime
left a comment
There was a problem hiding this comment.
Thanks for looking into this @LeeYoung624!
I think commits should be squashed or the effective commit rebased.
build/cmake/programs/CMakeLists.txt
Outdated
| install(FILES ${CMAKE_CURRENT_BINARY_DIR}/zstdcat DESTINATION "bin") | ||
| install(FILES ${CMAKE_CURRENT_BINARY_DIR}/unzstd DESTINATION "bin") | ||
| install(FILES ${CMAKE_SOURCE_DIR}/../../programs/zstdgrep DESTINATION "bin" PERMISSIONS OWNER_READ OWNER_WRITE OWNER_EXECUTE GROUP_READ GROUP_EXECUTE WORLD_READ WORLD_EXECUTE) | ||
| install(FILES ${CMAKE_SOURCE_DIR}/../../programs/zstdless DESTINATION "bin" PERMISSIONS OWNER_READ OWNER_WRITE OWNER_EXECUTE GROUP_READ GROUP_EXECUTE WORLD_READ WORLD_EXECUTE) |
There was a problem hiding this comment.
Perhaps there's too much toggling or permissions?
| install(FILES ${CMAKE_SOURCE_DIR}/../../programs/zstdless DESTINATION "bin" PERMISSIONS OWNER_READ OWNER_WRITE OWNER_EXECUTE GROUP_READ GROUP_EXECUTE WORLD_READ WORLD_EXECUTE) | |
| install(FILES ${CMAKE_SOURCE_DIR}/../../programs/zstdless DESTINATION "bin") |
There was a problem hiding this comment.
well, the default permissions is OWNER_WRITE, OWNER_READ, GROUP_READ, and WORLD_READ
cmake install
which means the users can not directly use zstdless and zstdgrep after make install.
Users need to chmod for zstdless and zstdgrep.
I tested on Ubuntu, Redhat, Centos and suse, and all of them show the same result if I didn't add the execute permissions
-bash: /usr/local/bin/zstdless: Permission denied
I think this should be done in cmake scripts.
And of course, the commits should be squashed. Thank you so much
There was a problem hiding this comment.
Can you explain why these permissions need to be explicitly set here, while they don't on the zstd install command? I guess it's because zstd is a TARGETS object, while these are FILES objects. Would it make more sense for these to be TARGETS themselves?
Or maybe these should be PROGRAMS?
The PROGRAMS form is identical to the FILES form except that the default permissions for the installed file also include OWNER_EXECUTE, GROUP_EXECUTE, and WORLD_EXECUTE. This form is intended to install programs that are not targets, such as shell scripts. Use the TARGETS form to install targets built within the project.
There was a problem hiding this comment.
@felixhandte I checked the camke documentation and ask some guys familiar with CMake. I think you are right. These should be PROGRAMS. Thank you so much.
It's re-synchronized with nextToUpdate at beginning of each block. It only needs to be tracked from within zstd_opt block parser. Made the logic clear, so that no code tried to maintain this variable. An even better solution would be to make nextToUpdate3 an internal variable of ZSTD_compressBlock_opt_generic(). That would make it possible to remove it from ZSTD_matchState_t, thus restricting its visibility to only where it's actually useful. This would require deeper changes though, since the matchState is the natural structure to transport parameters into and inside the parser.
it's now a local variable of ZSTD_compressBlock_opt()
as suggested by @terrelln. added <assert.h> include
felixhandte
left a comment
felixhandte
left a comment
There was a problem hiding this comment.
Looks good to me! Thanks!
|
@LeeYoung624, can you fix your stack so that it doesn't include rebases of commits already merged? |
|
Yes @felixhandte I will rebase this |
fix cmake bugs: didn't install zstdless and zstdgrep fix cmake bugs: cmake doesn't install zstdless and zstdgrep
|
@felixhandte seems I mess up the stack of this PR.. I will close thie PR and open a new one for this cmake fix. Sorry about this |
5 participants
As it's described here
CMake does not install zstdgrep and zstdless shell scripts
Copy zstdgrep and zstdless in CMake make install, and grant them execute permission