cuspatial icon indicating copy to clipboard operation
cuspatial copied to clipboard

Remove proj and sqlite from libcuspatial recipe.

Open bdice opened this issue 2 years ago • 4 comments

Description

This cleans up some packaging. We no longer need proj or sqlite in libcuspatial.

Checklist

  • [x] I am familiar with the Contributing Guidelines.
  • [x] New or existing tests cover these changes.
  • [x] The documentation is up to date with these changes.

bdice avatar Dec 06 '23 23:12 bdice

Also while doing that, we should consider pinning proj in dependencies.yaml (unless the migration is completed on cf prior to this PR merging, in which case we can remove the pinnings in this PR).

vyasr avatar Dec 08 '23 01:12 vyasr

My concern with removing the add_subdirectory(cuproj) line is that libcuproj will not be built. THe source structure is:

cpp
 | - CMakeLists.txt (builds libcuspatial, includes proj)
 |--> cuproj
           | - CMakeLists.txt

This is a bit confusing if the build process is separate. Perhaps we should change it to:

 |--> cuspatial
         | - CMakeLists.txt
 |--> proj
           | - CMakeLists.txt

But that's a pretty big change. Thoughts?

harrism avatar Dec 09 '23 21:12 harrism

@harrism I agree that there are varying degrees of scope that this could take. Let's limit the scope in this PR and restructure parts of dependencies.yaml but leave the rest of the refactor for another PR.

I think what I would propose for the second PR is:

  • Organize the C++ code to something like
cpp
 |--> cuspatial
         | - CMakeLists.txt
 |--> cuproj
         | - CMakeLists.txt
  • Consider adding a separate libcuproj package (with changes to build.sh, ci/build_cpp.sh, ci/build_python.sh, a new conda recipe, etc.)

libcuproj is header-only, so the conda package contents would be just headers (and it would define the build dependencies for those headers), like for rmm. It's also fine for us to just not have a libcuproj package and instead build the C++ code in the cuproj Python build steps, but the problem is really that libcuproj tests/benchmarks are being lumped with libcuspatial's build process, as far as I can tell. The need to isolate tests/benchmarks are enough for me to vote in favor of a separate conda package. I would still advocate for the C++ file organization above, either way.

bdice avatar Dec 09 '23 21:12 bdice

I don't think this PR is going to work until the packages are restructured as discussed in the above threads. Right now cuproj is build as part of cuspatial, so the cuproj requirements are needed for the cuspatial build. You may have to do the package restructuring in order to simplify dependencies the way that you want.

vyasr avatar Dec 12 '23 02:12 vyasr

Closing as stale. I do not plan to refactor the packages as described above in the near term.

bdice avatar Aug 05 '24 15:08 bdice