Skip to content

Reuse <version_config.h> instead of separate version constants in c_api.h#1003

Merged
rapids-bot[bot] merged 20 commits intorapidsai:branch-25.08from
mythrocks:c-api-version-bump
Jun 27, 2025
Merged

Reuse <version_config.h> instead of separate version constants in c_api.h#1003
rapids-bot[bot] merged 20 commits intorapidsai:branch-25.08from
mythrocks:c-api-version-bump

Conversation

@mythrocks
Copy link
Copy Markdown
Contributor

@mythrocks mythrocks commented Jun 10, 2025

This commit started off trying to fix an error in how version strings are replaced in the C API header. (Bumping the version to 25.08 causes the minor version to be written as "08" instead of "8", producing an invalid octal number.)

The octal problem was already solved as part of RAPIDS CMake, which is used in cuVS to generate a version_configi.h with the corrected version values. This commit removes the old version constants, and switches to using what's provided by infrastructure.

This commit fixes an error in how version strings are replaced in the C
API header.

Bumping the version to 25.08 causes the minor version to be written as
"08" instead of "8", producing an invalid octal number.

This commit fixes the error in the script, and also bumps the versions
in the C API, documentation, and workflow yamls.
@mythrocks mythrocks self-assigned this Jun 10, 2025
@mythrocks mythrocks requested review from a team as code owners June 10, 2025 23:27
@mythrocks mythrocks requested a review from bdice June 10, 2025 23:27
@mythrocks mythrocks added bug Something isn't working non-breaking Introduces a non-breaking change labels Jun 10, 2025
@mythrocks
Copy link
Copy Markdown
Contributor Author

That the yaml files needed updating on branch-25.08 came as a surprise. I'd appreciate if someone familiar with them reviewed that change closely.

Comment thread cpp/include/cuvs/core/c_api.h Outdated
@cjnolet
Copy link
Copy Markdown
Member

cjnolet commented Jun 12, 2025

@mythrocks

I think the update_version.sh does change the cmake files, doesn't it? I think the suggest was just to have cmake update the c header, rather than relying on update_versions to do it. Honestly, i could go either way with this, so it doesn't matter a ton to me- so long as we aren't forced to update anything manually.

@mythrocks
Copy link
Copy Markdown
Contributor Author

(I'll fix the copyright dates shortly.)

@mythrocks
Copy link
Copy Markdown
Contributor Author

I think the update_version.sh does change the cmake files, doesn't it?

Not directly. It seems to update the VERSION file, and then CMake does its bit based on the VERSION.

so long as we aren't forced to update anything manually.

I've gotten an idea of how @KyleFromNVIDIA would prefer this were fixed. I'll try incorporate that change ASAP.

@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot Bot commented Jun 13, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@mythrocks
Copy link
Copy Markdown
Contributor Author

It took me a little while to realize that what I'm trying to build here might be available already:

rapids_cmake_write_version_file(include/cuvs/version_config.hpp)

@mythrocks
Copy link
Copy Markdown
Contributor Author

I have verified that RAPIDS CMake line mentioned above creates build/include/cuvs/version_config.hpp with constants similar to the ones I introduced in c_api.h:

#define CUVS_VERSION_MAJOR 25
#define CUVS_VERSION_MINOR 8
#define CUVS_VERSION_PATCH 0

This file lands up also being CMake-installed into the final install directory.

But I can't seem to find a way to include the header in c_api.h:

/home/mithunr/workspace/dev/cuvs/3/cpp/include/cuvs/core/c_api.h:19:10: fatal error: cuvs/version_config.hpp: No such file or directory
   19 | #include <cuvs/version_config.hpp>
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~

I'll seek @KyleFromNVIDIA's guidance on how this needs to be included, for the first build.

@mythrocks mythrocks requested a review from a team as a code owner June 17, 2025 21:27
@github-actions github-actions Bot added the CMake label Jun 17, 2025
@mythrocks
Copy link
Copy Markdown
Contributor Author

But I can't seem to find a way to include the header in c_api.h:

I've modified the CMakeLists for libcuvs and its tests to find the header correctly. I think this should address what @KyleFromNVIDIA had in mind.

@mythrocks
Copy link
Copy Markdown
Contributor Author

/ok to test b973420

@mythrocks
Copy link
Copy Markdown
Contributor Author

@KyleFromNVIDIA, please let me know if this looks acceptable.

Copy link
Copy Markdown
Member

@KyleFromNVIDIA KyleFromNVIDIA left a comment

Choose a reason for hiding this comment

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

One quick change plus one small note and then I think we're good.

Comment thread cpp/tests/CMakeLists.txt Outdated
Comment thread cpp/tests/core/c_api.c Outdated
Copy link
Copy Markdown
Member

@KyleFromNVIDIA KyleFromNVIDIA left a comment

Choose a reason for hiding this comment

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

Actually left a few more comments, let me know what you think.

Comment thread cpp/include/cuvs/core/c_api.h
Comment thread cpp/src/core/c_api.cpp Outdated
rapids-bot Bot pushed a commit to rapidsai/rapids-cmake that referenced this pull request Jun 26, 2025
1. Removed include-dir settings from tests/CMakeLists.
2. Removed version header from c-api source and tests.
3. Added version header to c-api header.
4. Modified `jextract` script for the include directory, so as to find
   the version header.
@mythrocks
Copy link
Copy Markdown
Contributor Author

/ok to test ed14bb7

@mythrocks
Copy link
Copy Markdown
Contributor Author

/ok to test 0a98174

1 similar comment
@KyleFromNVIDIA
Copy link
Copy Markdown
Member

/ok to test 0a98174

@mythrocks
Copy link
Copy Markdown
Contributor Author

/ok to test af53e0e

@mythrocks
Copy link
Copy Markdown
Contributor Author

/ok to test bda2c67

@mythrocks
Copy link
Copy Markdown
Contributor Author

mythrocks commented Jun 26, 2025

@KyleFromNVIDIA: After including the cuvs/version_config.h header in c_api.h, I'm seeing a Java build failure:

c_api.h:19:10: error: 'cuvs/version_config.h' file not found

(This wasn't happening before, because the version header was included only in c_api.cpp, and not subject to jextract.)

I fixed this in my local builds by including cpp/build/include in the include paths. But it appears that the path used in CI builds is a different one.

By my reading, even if we were to use the right one for CI, it would still cause jextract to fail in local dev builds. Note that jextract is being run for the Java builds, and not from running cmake.

At this point, I'm inclined to change things back so that:

  1. <version_config.h> is included only in the c_api.cpp.
  2. Change the jextract code back to not depending on finding <version_config.h>.

Please let me know if that is agreeable.

@mythrocks
Copy link
Copy Markdown
Contributor Author

/ok to test 663013a

@mythrocks
Copy link
Copy Markdown
Contributor Author

mythrocks commented Jun 27, 2025

@jameslamb, @rhdong: I was wondering if either of you might be able to help here.

The problem is as follows:

  1. There is a header (<cuvs/version_config.h>) that is generated during the C++ build of libcuvs. This is now required in the Java build.
  2. I do know that on my workstation, cuvs/version_config.h is generated in $PROJECT_ROOT/cpp/build/include. I don't know what the location of this file is, in the C++ CI build. It doesn't seem to be in cpp/build/include.
  3. I have tried including "${REPODIR}"/cpp/build/include as an include-directory for the Java build. But the build still fails to find the header.
  4. Debugging the build, I ran a find for the header in $REPODIR. It isn't found.

May I please know in which directory the C++ build artifacts go? Also, what might be the best way to communicate that to the Java build?

(@KyleFromNVIDIA: This is a last-ditch attempt. If there isn't an easy/good way to resolve this, I'd very much like to include the version-header in the c_api.c file, and be done with it for now. We can do a follow-on PR, in case someone misses the version strings being in the c_api.h. That change was too recent for this to be a problem, I think.)

@KyleFromNVIDIA
Copy link
Copy Markdown
Member

If there isn't an easy/good way to resolve this, I'd very much like to include the version-header in the c_api.c file, and be done with it for now. We can do a follow-on PR, in case someone misses the version strings being in the c_api.h. That change was too recent for this to be a problem, I think.

That's fine with me, if you can't figure out another solution then go ahead and do it.

1. Removed version_config.h from c_api.h.
2. Restored inclusion of version_config.h to c_api.c and c_api.cpp.
3. Reverted modification to include paths for jextract.
@mythrocks
Copy link
Copy Markdown
Contributor Author

/ok to test fc631e1

@mythrocks mythrocks changed the title Fix update-version.sh to remove leading zeros. Reuse <version_config.h> instead of separate version constants in c_api.h Jun 27, 2025
@KyleFromNVIDIA
Copy link
Copy Markdown
Member

/merge

@rapids-bot rapids-bot Bot merged commit 88bf26a into rapidsai:branch-25.08 Jun 27, 2025
53 checks passed
@github-project-automation github-project-automation Bot moved this from In Progress to Done in Unstructured Data Processing Jun 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working ci CMake cpp non-breaking Introduces a non-breaking change

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants