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

[openvdb] Fix CUDA headers installation path #41102

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

[openvdb] Fix CUDA headers installation path #41102

wants to merge 3 commits into from
+80 −23

Conversation

loryruta
Copy link

The scope of this PR is to fix nanovdb compiled with CUDA.

The issue is that the following lines of code are evaluated when compiling with nvcc:

https://github.com/AcademySoftwareFoundation/openvdb/blob/6c044e628a1ac3546eed48b6f5e9c76dfaa2143e/nanovdb/nanovdb/util/GridHandle.h#L492

But the util/cuda/ directory wasn't correctly installed (all files went under the util directory).
Therefore the compiler was unable to find util/cuda/ files.

The issue is a openvdb v11.0.0 issue (latest stable version?), I'm not sure if persists in the upstream. See:

https://github.com/AcademySoftwareFoundation/openvdb/blob/6c044e628a1ac3546eed48b6f5e9c76dfaa2143e/nanovdb/nanovdb/CMakeLists.txt#L273

  • Changes comply with the maintainer guide.
  • SHA512s are updated for each updated download.
  • The "supports" clause reflects platforms that may be fixed by this new version.~
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • When updating the upstream version, the "port-version" is reset (removed from vcpkg.json).
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@loryruta
Copy link
Author

@microsoft-github-policy-service agree

@loryruta loryruta marked this pull request as ready for review September 21, 2024 15:04
@MonicaLiu0311
Copy link
Contributor

MonicaLiu0311 commented Sep 23, 2024 •
edited
Loading

Please commit ports/nanovdb/vcpkg.json, refer to the example: 40575/files.

maintainer guide#versioning.

@MonicaLiu0311 MonicaLiu0311 added the category:port-bug The issue is with a library, which is something the port should already support label Sep 23, 2024
@MonicaLiu0311 MonicaLiu0311 marked this pull request as draft September 23, 2024 03:13
@dg0yt
Copy link
Contributor

dg0yt commented Sep 23, 2024

Changing installation layout may break downstream usage. IIUC upstream's direction is to change the #include statements, not the installation layout. Git master:
https://github.com/AcademySoftwareFoundation/openvdb/blob/master/nanovdb/nanovdb/GridHandle.h#L489-L491

@loryruta
Copy link
Author

Changing installation layout may break downstream usage

Downstream usage doesn't exist/is broken because files were including Cuda* headers inside util/cuda that weren't installed. Now it's fixed

IIUC upstream's direction is to change the #include statements, not the installation layout

Yes, they moved CUDA files to cuda/ and tools/cuda/, but that will be another version, doesn't it? For the current the installation requires CUDA files to be at util/cuda/ (as they are in the original source code)

@loryruta loryruta changed the title [nanovdb] Fix CUDA headers installation path [openvdb] Fix CUDA headers installation path Sep 23, 2024
@loryruta
Copy link
Author

Please commit ports/nanovdb/vcpkg.json, refer to the example: 40575/files.

maintainer guide#versioning.

Why? nanovdb is a feature of openvdb. The port is openvdb and is already there 🤔

dg0yt
dg0yt reviewed Sep 23, 2024
View reviewed changes
Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

Downstream usage doesn't exist/is broken because files were including Cuda* headers inside util/cuda that weren't installed. Now it's fixed

There were installed to a different location. I am unable to tell if downstream usage might rely on this location directly.

IIUC upstream's direction is to change the #include statements, not the installation layout

Yes, they moved CUDA files to cuda/ and tools/cuda/, but that will be another version, doesn't it? For the current the installation requires CUDA files to be at util/cuda/ (as they are in the original source code)

Why should the headers be moved to a subdir only to move them back in the next release?
Too many #include to fix?

ports/openvdb/fix_nanovdb.patch
Comment on lines +14 to +28
@@ -169,17 +169,6 @@ set(NANOVDB_INCLUDE_UTILFILES
util/CpuTimer.h
util/CreateNanoGrid.h
util/CSampleFromVoxels.h
- util/cuda/CudaAddBlindData.cuh
- util/cuda/CudaDeviceBuffer.h
- util/cuda/CudaGridChecksum.cuh
- util/cuda/CudaGridHandle.cuh
- util/cuda/CudaGridStats.cuh
- util/cuda/CudaIndexToGrid.cuh
- util/cuda/CudaNodeManager.cuh
- util/cuda/CudaPointsToGrid.cuh
- util/cuda/CudaSignedFloodFill.cuh
- util/cuda/CudaUtils.h
- util/cuda/GpuTimer.h
Copy link
Contributor

Choose a reason for hiding this comment

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

This added the headers to NANOVDB_INCLUDE_UTILFILES.

ports/openvdb/fix_nanovdb.patch
+set(NANOVDB_INSTALL_UTILCUDADIR ${NANOVDB_INSTALL_UTILDIR}/cuda)

install(FILES ${NANOVDB_INCLUDE_FILES} DESTINATION ${NANOVDB_INSTALL_INCLUDEDIR})
install(FILES ${NANOVDB_INCLUDE_UTILFILES} DESTINATION ${NANOVDB_INSTALL_UTILDIR})
Copy link
Contributor

Choose a reason for hiding this comment

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

This installs NANOVDB_INCLUDE_UTILFILES.

@dg0yt
Copy link
Contributor

dg0yt commented Sep 23, 2024

Please commit ports/nanovdb/vcpkg.json, refer to the example: 40575/files.
maintainer guide#versioning.

Why? nanovdb is a feature of openvdb. The port is openvdb and is already there 🤔

What was meant is:

  1. Restore the data in versions.
  2. Add "port-version": "1", in ports/openvdb/vcpkg.json.
  3. Commit these changes.
  4. Run vcpkg x-add-version openvdb.
  5. Commit the new changes to versions.

@loryruta
Copy link
Author

There were installed to a different location. I am unable to tell if downstream usage might rely on this location directly.

I am unable to tell too. What I can say is that it's not the standard way of using nanovdb: if you look at the get started example or the examples you can see they use the /util/cuda path. So whoever is using e.g. /util/CudaDeviceBuffer.h (and not /util/cuda/CudaDeviceBuffer.h) is someone that understood that there is a flaw in the nanovdb installation

Why should the headers be moved to a subdir only to move them back in the next release?
Too many #include to fix?

They didn't really moved them back, they changed the code structure: some headers went to /cuda/, others to /tools/cuda/, and they deprecated everything inside /util/cuda. They refactored the #include directives too

@loryruta loryruta marked this pull request as ready for review September 23, 2024 14:21
@MonicaLiu0311 MonicaLiu0311 added the info:reviewed Pull Request changes follow basic guidelines label Sep 26, 2024
@JavierMatosD JavierMatosD added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Sep 26, 2024
@BillyONeal
Copy link
Member

Have you reported this issue and/or submitted this change upstream?

@loryruta
Copy link
Author

loryruta commented Sep 27, 2024 •
edited
Loading

Have you reported this issue and/or submitted this change upstream?

As far as I'm seeing it's fixed in the current OpenVDB upstream (also, the include directories structure has changed):

https://github.com/AcademySoftwareFoundation/openvdb/blob/master/nanovdb/nanovdb/CMakeLists.txt#L324-L340

@JavierMatosD
Copy link
Contributor

If this bug is fixed in upstreams master than we should either download the patch from upstream or wait until their next release. It looks like their last release was in November last year and they seem to mint releases at least once a year. Maybe it's worth reaching out and seeing if they are willing to make another release?

@JavierMatosD JavierMatosD marked this pull request as draft September 27, 2024 17:37
@loryruta
Copy link
Author

If this bug is fixed in upstreams master than we should either download the patch from upstream or wait until their next release. It looks like their last release was in November last year and they seem to mint releases at least once a year. Maybe it's worth reaching out and seeing if they are willing to make another release?

As the working patch for v11.0.0 is here and ready, I would currently merge this PR and think about the next version later; without waiting for it to come out. I can provide the port for the next version myself but I don't see to have much time in the near future

@JavierMatosD
Copy link
Contributor

As the working patch for v11.0.0 is here and ready, I would currently merge this PR and think about the next version later; without waiting for it to come out. I can provide the port for the next version myself but I don't see to have much time in the near future

If upstream has already fixed this bug, please download and apply the patch directly rather than checking it in.

@JavierMatosD JavierMatosD removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Oct 3, 2024
@MonicaLiu0311
Copy link
Contributor

@loryruta

@loryruta
Copy link
Author

loryruta commented Oct 8, 2024

If upstream has already fixed this bug, please download and apply the patch directly rather than checking it in.

This is the commit history of the file of interest: nanovdb/nanovdb/CMakeLists.txt:

https://github.com/AcademySoftwareFoundation/openvdb/commits/master/nanovdb/nanovdb/CMakeLists.txt

77f28d1 is the commit where the file is at, at our current version (that is v11.0.0).

The last two commits fix the bug, but at the same time depend on the new project structure (that e.g. adds the tools/ folder). It's not trivial for me to extract the changes.

So, as you stated in one of your messages (@JavierMatosD), we can try to wait for November to see if they will deploy another release, and make the port for it.

For @danrbailey (openvdb dev): do you have any plan to release soon-ish?

@MonicaLiu0311 MonicaLiu0311 removed the info:reviewed Pull Request changes follow basic guidelines label Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dg0yt dg0yt dg0yt left review comments

@MonicaLiu0311 MonicaLiu0311 MonicaLiu0311 approved these changes

Assignees

@MonicaLiu0311 MonicaLiu0311

Labels
category:port-bug The issue is with a library, which is something the port should already support
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@loryruta @MonicaLiu0311 @dg0yt @BillyONeal @JavierMatosD