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: Fix target includes pointing to the wrong directory #1052

Merged
merged 2 commits into from Feb 4, 2022
Merged

CMake: Fix target includes pointing to the wrong directory #1052

merged 2 commits into from Feb 4, 2022

Conversation

leonvictor
Copy link
Contributor

@leonvictor leonvictor commented Jan 19, 2022

This fixes the shared and static targets include directories, now pointing to lz4/lib rather than lz4/build/cmake

@Cyan4973
Copy link
Member

I'm trying to understand what's the matter to understand what is being fixed here.
Also, should the new behavior be checked by a test ?

@leonvictor
Copy link
Contributor Author

target_include_directories(target PUBLIC $<BUILD_INTERFACE: dir1 dir2>) is used to specify directories to include when building a given target (so roughly translates to -Idir1 -Idir2 args in gcc)

The previous behavior was to include CMAKE_CURRENT_SOURCE_DIR which points to the location of the current cmake file, in this case lz4/build/cmake. This PR changes that to use LZ4_SOURCE_DIR instead, which points to our source directory.

Actually this was already the behavior in the current release. This was changed with #1030 and it's probably a mistake due to the PR focusing on installation rather than build targets.

I hope this clarifies things !

As for testing, it looks like the cmake job compiles with make rather than CMake --build. I'm a bit clueless about the use-cases of other build methods, but we could add a new job actually buildind through CMake ?

@Cyan4973
Copy link
Member

Cyan4973 commented Feb 1, 2022

Thanks for the clarifications @leonvictor !

As for testing, it looks like the cmake job compiles with make rather than CMake --build. I'm a bit clueless about the use-cases of other build methods, but we could add a new job actually buildind through CMake ?

I believe you are correct, and the CI test should rather use cmake --build.
Do you want to bundle that change in this PR ?
Alternatively, I could do it in another commit.

@leonvictor
Copy link
Contributor Author

Hey, it looks like I'm not authorized to make change to the CI, even on my fork. I've never used github workflows so I might be missing something, but it's alright if you can do it yourself 😄

@Cyan4973 Cyan4973 merged commit e9c29be into lz4:dev Feb 4, 2022
@Cyan4973 Cyan4973 mentioned this pull request Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants