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.

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

[upstream_utils] Add Catch2 #6951

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

spacey-sooty
Copy link
Contributor

Resolves #6946

@spacey-sooty spacey-sooty force-pushed the catch2upstreamutil branch 6 times, most recently from 8a74859 to 7a84460 Compare August 13, 2024 06:22
@spacey-sooty
Copy link
Contributor Author

How do we wanna approach the Gradle build for this?
Does Gradle have a configure_file equivalent?

@spacey-sooty
Copy link
Contributor Author

Wait how is gradle passing 💀

@spacey-sooty spacey-sooty force-pushed the catch2upstreamutil branch 3 times, most recently from 0e6710b to 550e141 Compare August 14, 2024 01:42
@spacey-sooty
Copy link
Contributor Author

I don't think what I've done so far address the #cmakedefines

@PeterJohnson
Copy link
Member

PeterJohnson commented Aug 14, 2024

Well, I don't think we need to globally support the cmakedefine's, only what we think we'll actually want to customize in the gradle config. What you have is likely good enough.

@spacey-sooty spacey-sooty marked this pull request as ready for review August 14, 2024 14:31
@spacey-sooty spacey-sooty requested a review from a team as a code owner August 14, 2024 14:31
@spacey-sooty
Copy link
Contributor Author

Well, I don't think we need to globally support the cmakedefine's, only what we think we'll actually want to customize in the gradle config. What you have is likely good enough.

In that case this is ready for review 🙂

@PeterJohnson
Copy link
Member

PeterJohnson commented Aug 14, 2024

I think it would be good to add in this PR a simple catch2 test in wpiutil to check to make sure it actually works and hooks into the build the way we want.

@spacey-sooty
Copy link
Contributor Author

spacey-sooty commented Aug 15, 2024

The tests wont run on Gradle cause we dont have a Catch2 test runner (I don't think that should be blocking for this pr though)

@spacey-sooty spacey-sooty force-pushed the catch2upstreamutil branch 3 times, most recently from 795bcd5 to 6b00dc7 Compare August 15, 2024 05:24
thirdparty/catch2/build.gradle Outdated Show resolved Hide resolved
thirdparty/catch2/build.gradle Outdated Show resolved Hide resolved
@spacey-sooty spacey-sooty force-pushed the catch2upstreamutil branch 2 times, most recently from f98265b to d5d63eb Compare August 16, 2024 01:17
Copy link
Contributor

@Gold856 Gold856 left a comment

Choose a reason for hiding this comment

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

Try copying googletest.gradle and swapping out the googletest references with catch2. Then apply the new catch2 file the same way the googletest file is applied. I don't think we actually use Gradle's googletest-specific integrations, since I think we just run the test executable and check for exit code 0. If that's the case, we can pretend like our Catch2 tests are "GoogleTest" tests, and Gradle shouldn't have a problem with running them.

@spacey-sooty
Copy link
Contributor Author

We use GoogleTestTestSuiteBinarySpec which is part of Gradle's integration?

@spacey-sooty
Copy link
Contributor Author

spacey-sooty commented Aug 27, 2024

Ohh I think I see the issue its like the dumb protobuf bs

thirdparty/catch2/build.gradle Outdated Show resolved Hide resolved
thirdparty/catch2/publish.gradle Outdated Show resolved Hide resolved
thirdparty/catch2/publish.gradle Outdated Show resolved Hide resolved
thirdparty/catch2/publish.gradle Outdated Show resolved Hide resolved
thirdparty/catch2/build.gradle Outdated Show resolved Hide resolved
@Gold856
Copy link
Contributor

Gold856 commented Aug 28, 2024

Looks like you're going to have to patch the user config header to not have #cmakedefine. How you handle that is up to you (use an ifndef and a define for Gradle, removing them entirely, etc.)

@spacey-sooty
Copy link
Contributor Author

I guess it wouldn't be too hard to just replace every cmakedefine with a define...

@spacey-sooty
Copy link
Contributor Author

Gradle hates me

thirdparty/catch2/build.gradle Outdated Show resolved Hide resolved
upstream_utils/catch2.py Outdated Show resolved Hide resolved
@github-actions github-actions bot added the component: wpiutil WPI utility library label Dec 19, 2024
Signed-off-by: Jade Turner <[email protected]>
@Gold856
Copy link
Contributor

Gold856 commented Dec 19, 2024

Probably want to update this to use the latest commit of Catch2 as well.

Signed-off-by: Jade Turner <[email protected]>
@Gold856
Copy link
Contributor

Gold856 commented Dec 19, 2024

I think the solution to the config header is to just clone the repo, configure the build with CMake, and copy the generated header into our source tree. Bonus points if you can set up a generation script for it.

@spacey-sooty
Copy link
Contributor Author

I think the solution to the config header is to just clone the repo, configure the build with CMake, and copy the generated header into our source tree. Bonus points if you can set up a generation script for it.

I took a look at the generated file, we actually only need to replace two options (the one with values). The rest can be commented out and it'll use the default. I think if we can do that properly it'll be fine.

Signed-off-by: Jade Turner <[email protected]>
Signed-off-by: Jade Turner <[email protected]>
Signed-off-by: Jade Turner <[email protected]>
Signed-off-by: Jade Turner <[email protected]>
Signed-off-by: Jade Turner <[email protected]>
Signed-off-by: Jade Turner <[email protected]>
Signed-off-by: Jade Turner <[email protected]>
This reverts commit 39e677b.
Signed-off-by: Jade Turner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: wpiutil WPI utility library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Catch2 to upstream_utils
3 participants