Nvcomp revert followup

[revans2]

This includes rework from #2483 that didn't make it into 24.10 and an updated version of the patch. The old patch would not apply cleanly in 24.12. The new patch was created by applying the old one to CUDF in 24.10, and then up-merging that branch to 24.12.

[revans2]

build

[revans2]

I ran the full set of spark-rapids tests with this and it looks like everything passes.

[gerashegalov]

Currently we fail the 2nd+ build if we don't add clean every time.

[INFO]      [exec] ~/gits/NVIDIA/spark-rapids-jni.reviews/thirdparty/cudf ~/gits/NVIDIA/spark-rapids-jni.reviews
[INFO]      [exec] Error: CUDF repository has uncommitted changes. No patches will be applied...
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 0.790 s
[INFO] Finished at: 2024-10-11T18:27:43Z
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-antrun-plugin:3.0.0:run (cudf patch) on project spark-rapids-

We could instead after, all patches successfully apply initially, generate a cumulative.patch somewhere under target.
and check if git -C thirdparty/cudf apply --check --reverse target/cumulative.patch succeeds. If yes, skip applying patches,

[mythrocks]

Thanks for fixing that, @revans2.

[gerashegalov]

LGTM already, the comment about detecting of prior patch applications can be another PR

[jlowe]

Curious why we're patching regcomp and string extract files as part of fixing cudf to have nvcomp 3.x?

[jlowe]

Since 24.12 build is totally busted, filed #2498 to track this issue. It's pretty nasty, since it silently doesn't build any changes. Running with outdated binaries relative to the source after a successful build is confusing to debug.

[jlowe]

There's no changes to thirdparty/cudf-pins/versions.json to have nvcomp be the old version, so I'm not sure this does what is intended.

[revans2]

I am not sure what I messed up but the patch is not good. I am working on a fix for it.

[revans2]

Sorry @gerashegalov @mythrocks and @jlowe I messed up when making the patch. I was multitasking and ended up putting up a patch file that was for the wrong thing, but because the versions file was pointing at the new version of nvcomp, then in order for it to pass we just had to get the patch to apply and compile. So it was two wrongs making a right. I think I have that fixed now.

[revans2]

Or I should say two wrongs making something that compiled, but was still wrong.

[revans2]

build

[jlowe]

This looks a lot better. No major changes to the patch file and the pin versions file is being updated to pull in the old nvcomp version.

This is a bit hostile to developers since you cannot build twice in a row without remembering to specify -Dsubmodule.patch.skip=true after the first build. Would like to see this be updated to automatically detect whether the patch has been applied or not if we're going to do this long-term.

[jlowe]

Why are we skipping building the C++ code if we're skipping patching? This prevents building with subsequent changes on a followup build that refuses to proceed if it's already been patched.

[revans2]

@gerashegalov this was your suggestion. I assume that it was to avoid building CUDF in an unpatched state.

[gerashegalov]

I tried to get rid of unnecessary work in the validate phase of the original PR assuming that we don't want to build anything unless the repo is patched.

[jlowe]

Approving to unblock 24.12 builds.

[revans2]

I have some more work to do to make this okay for development that I will do next week. Sorry to everyone who is having problems with this until then.