libQGLViewer icon indicating copy to clipboard operation
libQGLViewer copied to clipboard

Enabled CI MSVC build jobs.

Open bjornpiltz opened this issue 1 year ago • 10 comments

  • Added a build matrix
  • Installed Qt through jurplel/install-qt-action (allows for caching)

bjornpiltz avatar Oct 11 '23 20:10 bjornpiltz

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.

dennis2society avatar Oct 11 '23 21:10 dennis2society

@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.

bjornpiltz avatar Oct 11 '23 21:10 bjornpiltz

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.

dennis2society avatar Oct 11 '23 21:10 dennis2society

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.

bjornpiltz avatar Oct 11 '23 22:10 bjornpiltz

Perhaps @fghoussen can resubmit once this gets merged.

Sure!

fghoussen avatar Oct 12 '23 07:10 fghoussen

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.

dennis2society avatar Oct 12 '23 09:10 dennis2society

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

fghoussen avatar Oct 12 '23 09:10 fghoussen

@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.

bjornpiltz avatar Oct 12 '23 11:10 bjornpiltz

@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.

dennis2society avatar Oct 12 '23 11:10 dennis2society

@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!

fghoussen avatar Oct 12 '23 11:10 fghoussen