fix: Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests

[vyasr]

Resolves #350. Resolves #400.

Happy to reorganize things.

[paleolimbot]

Thank you! I will give this a review today and perhaps bring in some CMake expertise from previous contributors 🙂

[vyasr]

The example code here is an expanded version of what's in the CMake minimal example. I don't know if you still want to keep both around, I'll leave that up to you.

This PR is now ready for another round of review, I've addressed everything that was discussed I think.

[paleolimbot]

The example code here is an expanded version of what's in the CMake minimal example

This is OK, but I wonder if just:

int main() {
  return ArrowNanoarrowCheckRuntime();
}

...would be sufficient to check what you've written here. We can always expand the example later but I think keeping it relevant to the includes and symbols at hand is probably best.

[vyasr]

The example code here is an expanded version of what's in the CMake minimal example

This is OK, but I wonder if just:

int main() {
  return ArrowNanoarrowCheckRuntime();
}

...would be sufficient to check what you've written here. We can always expand the example later but I think keeping it relevant to the includes and symbols at hand is probably best.

I think it would be good to include something from both the C and C++ headers. So maybe also just a nanoarrow::UniqueArray tmp; declaration would suffice?

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 88.74%. Comparing base (dc50114) to head (3d156f9).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #406 +/- ## ======================================= Coverage 88.74% 88.74% ======================================= Files 81 81 Lines 14398 14398 ======================================= Hits 12778 12778 Misses 1620 1620 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[paleolimbot]

I think it would be good to include something from both the C and C++ headers. So maybe also just a nanoarrow::UniqueArray tmp; declaration would suffice?

That's a great point, and we should probably use it to do something to make extra sure it isn't optimized out. Maybe:

int main() {
  nanoarrow::UniqueSchema schema;
  NANOARROW_RETURN_NOT_OK(ArrowSchemaInitFromType(schema.get(), NANOARROW_TYPE_INT32));
  printf("Schema format for int32 is '%s'", schema->format);
  return EXIT_SUCCESS;
}

[vyasr]

You have it marked as a TODO already, but make sure the FetchContent call doesn't point to your fork.

I'll wait until I have approvals from both of you and then swap this out. Once I make that change CI won't pass on this PR, so I think we should plan to see a green CI run with all the changes that you both want in place, then I can make that final change and you can force merge as an admin.

[paleolimbot]

Whoops, I should have waited for CI. Pending green CI it's good to go!

[vyasr]

My bad, forgot to update the CI job.

[vyasr]

Cool, thanks to you both for the reviews! I've pushed one last commit changing the FetchContent call to point back to the main branch. We should be good to merge now.