GillesDebunne / libQGLViewer

libQGLViewer is an open source C++ library based on Qt that eases the creation of OpenGL 3D viewers.
Other
241 stars 94 forks source link

Enabled CI MSVC build jobs. #80

Open bjornpiltz opened 11 months ago

bjornpiltz commented 11 months ago
dennis2society commented 11 months ago

Would you consider to add this line to your #idfef(MSVC) block to suppress all the "inconsistent dll linkage" warnings as proposed in PR https://github.com/GillesDebunne/libQGLViewer/pull/79?

  # remove warnings about deprecation (CRT, dll linkage,etc) 
   target_compile_options(QGLViewer PRIVATE "/wd4996")

I agree that your ifdef(MSVC) is probably a better solution than the ifdef(WIN32) used in my PR.

bjornpiltz commented 11 months ago

@dennis2society I fixed the 4996 warnings by defining CREATE_QGLVIEWER_DLL, which was missing. see 20f8aa5a1d9e6ab5f1554b290c27ebe1d858347d.

As of now, building libQLViewer as a DLL is hard coded in the cmake system. It's up to @GillesDebunne to decide if the possibility to build it as a static library should be reintroduced.

Let me know if there is something else essential in your PR which is still missing (I think we can wait with fixing warnings and whitespace issues) otherwise this PR replaces #71 and #79.

dennis2society commented 11 months ago

You're right. I've looked at the output of your CI run and it looks fine. Possibly I have double-fixed this issue in my PR. I will test again with your CMakelists. Looks like you have everything covered. Nice work. I will close my own PR, no need to have two solutions for the same problem. And yours covers a bit more than mine already. Since you're at it, maybe you could also integrate https://github.com/GillesDebunne/libQGLViewer/pull/76. It is only a few lines long.

bjornpiltz commented 11 months ago

I think I'd prefer not to add #76 to keep this PR minimal. Anyway, I think a cleaner implementation would be:

if(QGLVIEWER_BUILD_EXAMPLES)
    add_subdirectory(examples) # details in examples/CMakeLists.txt
endif() 

Perhaps @fghoussen can resubmit once this gets merged.

fghoussen commented 11 months ago

Perhaps @fghoussen can resubmit once this gets merged.

Sure!

dennis2society commented 11 months ago

I have tested your CMakeLists and it works fine for me. Suppressing the 4996 warnings on Windows is not necessary. I will close my own PR.

fghoussen commented 11 months ago

So #76 will be merged? If #76 should be modified, just tell me how :)

bjornpiltz commented 11 months ago

@dennis2society, I'm hesitant to do so since I don't understand the changes made in "Minor tweaks in cmake configuration" I don't know why the minimum CMake version was raised as high as 3.16 and the cmake_policy() call was added. I would have thought the following two lines would be enough.

cmake_minimum_required(VERSION 3.0)
project(libQGLViewer LANGUAGES CXX VERSION 2.9.1)

@fghoussen, I'm hoping Gilles will merge this PR - then you would need to amend your branch to resolve the conflicts with my changes.

dennis2society commented 11 months ago

@bjornpiltz The reason I am suggesting this is this warning when running cmake:

CMake Warning (dev) at CMakeLists.txt:5 (project):
cmake_minimum_required() should be called prior to this top-level project()
call.  Please see the cmake-commands(7) manual for usage documentation of
both commands.

Not sure about the CMake version requirements, though.

fghoussen commented 11 months ago

@fghoussen, I'm hoping Gilles will merge this PR - then you would need to amend your branch to resolve the conflicts with my changes.

OK!