Skip to content

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

Merged
Cyan4973 merged 2 commits intolz4:devfrom
leonvictor:dev
Feb 4, 2022
Merged

CMake: Fix target includes pointing to the wrong directory#1052
Cyan4973 merged 2 commits intolz4:devfrom
leonvictor:dev

Conversation

@leonvictor
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

2 participants