search

SCIInstitute / SCIRun

SCIRun is a Problem Solving Environment, for modeling, simulation and visualization of scientific problems. This is version 5, the upgraded version of SCIRun v4.
http://scirun.org
Other
128 stars 72 forks source link

OSPRay Upgrade to 2.10 #2428

Closed tarkpate closed 1 year ago

tarkpate commented 1 year ago

Links to git tag from the PR branch for testing purposes. After that PR is merged, I'll revert the git repo back to cibc-internal.

Also, I added a include string statement in Mutex. I was getting errors on both of my machines that it was missing when I tried to build with OSPRay.

jcfr commented 1 year ago

Since origin/scirun-build-2.10 is a branch, I would instead suggest to do the following:

SET(ospray_GIT_TAG "29a6d246370fd3ec208d3668c84240403ab45ba9") # scirun-build-2.10

That say, different build of scirun will be ensured to build against the same version of ospray. That approach should likely be used for all dependencies.

Also, if possible, I suggest to submit pull request to the upstream ospray project and then backport the corresponding commit into the CIBC-Internal fork prefixing it with [Backport MR-NNNN] where NNNN is the pull request number.

dcwhite commented 1 year ago

I can make a named tag for our 2.10 version. There was no other activity on those repo copies, so no "danger" in using branches instead of tags. I don't think Ospray wants any of our changes (like deleting all the documentation), why would we send them a PR?

jcfr commented 1 year ago

I don't think Ospray wants any of our changes

The following could be done:

  1. Create an issue upstream explaining that having a sub-module to include the documentation leads to additional work when integrating in client project like SCIRun. You could suggest the following:

    • Remove the gitmodules as it doesn't seems to exist.
    • Use the FetchContent introduced in CMake 3.11. Since the CMake minimum required version in OSPRay is CMake 3.1, a fallback can easily be added to support older version.

  2. Submit a pull-request to rename include build_ospray instead of build_ospray.cmake as well as using CMAKE_CURRENT_LIST_DIR instead of CMAKE_CURRENT_SOURCE_DIR

  3. OSPRAY_INSTALL_DEPENDENCIES should probably be set before including OSPRay in SCIRun instead of being hard-coded in the project itself. This will ensure that building against another version of OSPRay will work as expected.

  4. Instead of modifying OSPRay to define the project SCIOspray, it may be worth adding a folder called SCIOspray in SCIRun. The idea would be to minimize updates to the dependency and streamline future updates.