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.

Sign up for GitHub

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

Jump to bottom

Add CI support, along with some fixes for Windows. #83

Merged
merged 1 commit into from
Sep 12, 2020
Merged

Add CI support, along with some fixes for Windows. #83

merged 1 commit into from
Sep 12, 2020

Conversation

Anteru
Copy link
Contributor

@Anteru Anteru commented Aug 31, 2020

Other changes:

  • Replace OpenGL::OpenGL with OpenGL::GL, as OpenGL::OpenGL doesn't work on Windows.
  • Use ZLIB::ZLIB to reference ZLIB, and add it to the public dependencies.
  • Disable shared libraries by default. The library has no symbol visibility set, so on Visual C++ it will not produce a library by default as there are no symbols.

I'm not 100% certain about the OpenGL::GL change, and I can easily make that conditional based on Windows if that's preferred, but I don't see use of EGL either so ... it's unclear if OpenGL::OpenGL is really needed. For the shared library, the correct fix would be to export all symbols that could be used by clients, but I'm not sure which ones those are so I'll rather err on the side of caution here.

davvid
davvid reviewed Sep 11, 2020
View reviewed changes
.github/workflows/build.yml Outdated
on:
push:
pull_request:
branches: [ master ]
Copy link
Member

Choose a reason for hiding this comment

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

main

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

davvid
davvid requested changes Sep 11, 2020
View reviewed changes
Copy link
Member

@davvid davvid 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 the CI scripts, this is really nice.

Please address these notes and rebase. When doing so, please also add a signed-off-by line (git commit --amend -s or ctrl-i in Git Cola) to your commit message that certifies that you wrote or otherwise have the right to pass on these changes on as an open-source patch.

Adding a signoff certifies that you adhere to https://developercertificate.org/

Thanks

CMakeLists.txt Outdated
@@ -34,7 +34,7 @@
cmake_minimum_required(VERSION 3.15.0)
project(partio LANGUAGES CXX)

option(BUILD_SHARED_LIBS "Enabled shared libraries" ON)
option(BUILD_SHARED_LIBS "Enabled shared libraries" OFF)
Copy link
Member

Choose a reason for hiding this comment

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

We're better off passing -D BUILD_SHARED_LIBS=OFF when invoking cmake via CI. Please assume a Linux/Unix environment. Windows-isms should be opt-in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. Done!

src/lib/CMakeLists.txt
$<INSTALL_INTERFACE:${ZLIB_INCLUDE_DIR}>
$<BUILD_INTERFACE:${ZLIB_INCLUDE_DIR}>)
target_link_libraries(partio PUBLIC ${ZLIB_LIBRARY})
target_link_libraries(partio PUBLIC ZLIB::ZLIB)
Copy link
Member

Choose a reason for hiding this comment

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

Thanks, the new target-based stuff is much nicer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

src/tools/CMakeLists.txt Outdated
@@ -41,7 +41,7 @@ if (GLUT_FOUND AND OPENGL_FOUND)
PRIVATE
${PARTIO_LIBRARIES}
GLUT::GLUT
OpenGL::OpenGL
OpenGL::GL
Copy link
Member

Choose a reason for hiding this comment

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

Hmm.. we're relying on the use of libglvnd to handle all of our vendor-specific GL stuff. Can you install glvnd in the CI builders so that OpenGL::OpenGL keeps working? If not, then a if (WIN32) ... conditional can be used to set a variable that lets us keep using OpenGL::OpenGL.

This could also be made into another option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a new option PARTIO_USE_GLVND which is OFF by default on Windows, and ON everywhere else.

@Anteru
Add CI support.
3d7cf9e 
Other changes:

* Add a new option PARTIO_USE_GLVND which is OFF by default on
  Windows. This option forces the use of OpenGL::GL over OpenGL::OpenGL
* Use ZLIB::ZLIB to reference ZLIB, and add it to the public
  dependencies.
* Rename GTEST_ENABLED to PARTIO_GTEST_ENABLED

Signed-off-by: Matthäus G. Chajdas <dev@anteru.net>
@Anteru
Copy link
Contributor Author

Anteru commented Sep 12, 2020

Thanks for the feedback. I've addressed it (hopefully all of it) and made one more small change, renaming GTEST_ENABLED to PARTIO_GTEST_ENABLED to not pollute the global namespace. Would you be ok with doing the same for BUILD_SHARED_LIBS? I.e. make that PARTIO_BUILD_SHARED_LIBRARY with the same defaults as for PARTIO_USE_GLVND?

davvid added a commit that referenced this pull request Sep 12, 2020
@davvid
build: gtest typofix and updates to use PARTIO_GTEST_ENABLED
b1163a9 
Related-to: #83
@davvid davvid merged commit 6b2072e into wdas:main Sep 12, 2020
@davvid
Copy link
Member

davvid commented Sep 12, 2020

Thanks again, nicely done. It looks like I introduced a typo in CMakeLists.txt a bit ago related to GTEST_LOCATION which I fixed up in b1163a9. I did also tweak the Makefile to account for the new PARTIO_GTEST_ENABLED.

Regarding BUILD_SHARED_LIBS -- it's probably best to leave that variable as-is. That variable is a cmake built-in, so it makes sense to continue using it for consistency/familiarity with other projects.

@shdwdln
Copy link

shdwdln commented Sep 12, 2020

hey Im glad I saw this!
I am building this on win64 right now and it is building partio.dll fine but it won't build partio.lib.
It is building partio.dll, partio.ilk, and partio.pdb.
Why would this be?

@shdwdln
Copy link

shdwdln commented Sep 12, 2020

nvm I see the problem.

@Anteru
Copy link
Contributor Author

Anteru commented Sep 12, 2020 •
edited
Loading

Nicely done spotting that typo. It somehow didn't cause a problem, so I didn't notice it at all. And thanks for the merge!

The reason I was suggesting to prefix it with PARTIO_ is to make it easier to use partio as a subproject in a bigger CMake project, there I may want to use BUILD_SHARED_LIBS elsewhere, and unfortunately option names are global in CMake.

@davvid
Copy link
Member

davvid commented Sep 12, 2020

That would be a nice option to add, thanks for bringing up the subproject use case.

@Anteru
Copy link
Contributor Author

Anteru commented Sep 13, 2020

I'll prepare a PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davvid davvid davvid requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Anteru @davvid @shdwdln @danfe