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

Make pmp and pmp_vis usable as a git submodule / cmake subdirectory #155

Closed

Conversation

mokafolio
Copy link
Contributor

Description

Replaced all include_directories calls in the cmake files with target_include_directories so that the targets can properly be used with cmake add_subdirectory and properly propagate include directories. This allows to you add pmp as a git submodule to your project. I tested my changes on osx, Ubuntu 22.04 and Windows 11 for both local and install builds.

Motivation

Because using pmp as a git submodule/cmake subdirectory makes it a lot more versatile.

Benefits

See above.

Drawbacks

N/A

Applicable Issues

N/A

@dsieger
Copy link
Member

dsieger commented May 18, 2023

Looks like a good step forward to improve our CMake code. I'd like to do some more manual testing. Similar as with the other PR: Please sign off your commits with a DCO so that we can include your changes.

Thanks!

@coveralls
Copy link

coveralls commented May 18, 2023

Coverage Status

Coverage: 92.607%. Remained the same when pulling 00e2b35 on mokafolio:moka/cmake_improvements into b5332d8 on pmp-library:main.

…ctories when used as a cmake submodule

Signed-off-by: Matthias Dörfelt <hi@mokafolio.de>
@dsieger
Copy link
Member

dsieger commented May 23, 2023

Quick update: I had a look and there are some issues that need fixing. In particular, it breaks the cross-compilation to JavaScript using emscripten. This is due to linking imgui with glew directly. I'll check how I can include additional changes in the PR.

@dsieger dsieger self-requested a review May 23, 2023 21:16
@dsieger dsieger self-assigned this May 23, 2023
@mokafolio
Copy link
Contributor Author

Sounds great, that was the one thing I didn't check!

@dsieger dsieger closed this in 4625b47 Aug 16, 2023
@dsieger
Copy link
Member

dsieger commented Aug 16, 2023

There were too many conflicts to resolve this easily. I therefore squashed your changes and my fixes into a single commit 4625b47 . Hope that's OK and please let me know if this is sufficient for your use case.

@mokafolio
Copy link
Contributor Author

Just tested it and it seems to work, thanks!

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

3 participants