jupyter-xeus / xeus-octave

Jupyter kernel for GNU Octave
https://xeus-octave.readthedocs.io/
GNU General Public License v3.0
57 stars 10 forks source link

Fix for osx #40

Closed marimeireles closed 3 years ago

marimeireles commented 3 years ago

Hi @rapgenic, I had to make these small changes to be able to build it locally for osx.

marimeireles commented 3 years ago

Should we release a new version with your changes so I can test it on conda-forge?

marimeireles commented 3 years ago

Should we release a new version with your changes so I can test it on conda-forge?

I just created patches for it, so no need anymore for now.

rapgenic commented 3 years ago

Should we release a new version with your changes so I can test it on conda-forge?

Will release when this is solved!

rapgenic commented 3 years ago

I'm sorry, but I still believe that the way I wrote it, at least in theory, is right, whereas your modifications do not really make sense to me:

GLFW is always a dependency, so the

target_link_libraries(${EXECUTABLE_NAME} glfw)

line should be outside the if that handles the opengl/osmesa choice.

OpenGL/Osmesa are mutually exclusive, so one branch of the if should only link OpenGL where the other should only link osmesa (as it was before):

# This calls "find_package(glfw3)", no need to call it again
find_or_fetch_package(glfw3 ...)

# This if handles the Opengl/Osmesa choice which is chosen independently from GLFW
if (GLFW3_OSMESA_BACKEND)
  pkg_check_modules(osmesa REQUIRED IMPORTED_TARGET osmesa)
  target_link_libraries(${EXECUTABLE_NAME} PkgConfig::osmesa)
  set(NOTEBOOK_TOOLKIT_CPU TRUE)
else()
  find_package(OpenGL REQUIRED)
  target_link_libraries(${EXECUTABLE_NAME} OpenGL::OpenGL)
endif()

# The GLFW linking is always done here because it's independent from the previous choice
target_link_libraries(${EXECUTABLE_NAME} glfw)

You put a find_package(glfw3) inside that if, which in my opinion is wrong, because glfw has already been found or downloaded by find_or_fetch call before.

As of now, then, I cannot merge it, at least until I've understood what's the problem with my original code...

I've fixed the OpenGL::GL target, which I'd mistakenly wrote OpenGL::OpenGL, so if you don't mind, could you please try with the new version in master, and if it still won't work, share with me the cmake logfiles?

marimeireles commented 3 years ago

Right, I'll test it on macos and paste the problem here. I cannot do it now, it will take a while. Thanks for the review.

rapgenic commented 3 years ago

I believe this is no longer necessary, thanks @marimeireles!