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 bug fix: didn't install zstdless and zstdgrep. #1647

Merged
merged 1 commit into from
Jun 15, 2019
Merged

CMake bug fix: didn't install zstdless and zstdgrep. #1647

merged 1 commit into from
Jun 15, 2019

Conversation

LeeYoung624
Copy link
Contributor

@LeeYoung624 LeeYoung624 commented Jun 11, 2019

CMake bug fix: CMake didn't install zstdless and zstdgrep.

I mess up the last PR#1625 and create a new PR here.
Really sorry about this. @felixhandte

@Mno-hime
Copy link

The commit itself might be improved, I suggest something like this:

Install zstdless and zstdgrep as 'programs'

Closes #1503.

@LeeYoung624
Copy link
Contributor Author

@Mno-hime Do you mean that the commit message should be more specific?

@Mno-hime
Copy link

Yes, subject of the pull request is ok, but the commit message can be more specific about what's happening in the commit.

@LeeYoung624
Copy link
Contributor Author

LeeYoung624 commented Jun 11, 2019

Sure, I think you are right. Thank you so much @Mno-hime .

Copy link
Contributor

@felixhandte felixhandte left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the stack! I have one last suggested change: it would be cleaner if you could use ${PROGRAMS_DIR}/ rather than ${CMAKE_SOURCE_DIR}/../../programs/. Will that work? And if so, can you switch to that?

Thanks!

@LeeYoung624
Copy link
Contributor Author

LeeYoung624 commented Jun 12, 2019

Sure. I have switched to this.

@Cyan4973
Copy link
Contributor

Thanks @LeeYoung624

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.

5 participants