Skip to content

Conversation

@r1viollet
Copy link
Contributor

@r1viollet r1viollet commented Jul 24, 2025

What does this PR do?:

Remove gtest during build
Clarify outputs for async-profiler check

Motivation:

Consecutive builds are faster

Additional Notes:

NA

How to test the change?:

We already call test commands which will run gtests

For Datadog employees:

  • If this PR touches code that signs or publishes builds or packages, or handles
    credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.
  • JIRA: [JIRA-XXXX]

Unsure? Have a question? Request a review!

Clarify outputs for async-profiler checkout
@r1viollet r1viollet requested a review from jbachorik July 24, 2025 13:00
@github-actions
Copy link

github-actions bot commented Jul 24, 2025

🔧 Report generated by pr-comment-scanbuild

@r1viollet r1viollet force-pushed the r1viollet/remove_gtest_during_build branch from 020f9cc to 964e71f Compare July 24, 2025 13:01
@github-actions
Copy link

github-actions bot commented Jul 24, 2025

🔧 Report generated by pr-comment-cppcheck

CppCheck Report

Errors (2)

Warnings (8)

Style Violations (300)

Copy link
Collaborator

@jbachorik jbachorik left a comment

Choose a reason for hiding this comment

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

Is it ok to run gtest only when testing Java?
You could then do linkDebug and get *.so file without running gtests. Is this the intention?

For CI I think this does make any difference - the gtests will run a bit later. For local dev, you can use -Pskip-gtest gradle paramater.

The rest of the changes looks good.

@r1viollet
Copy link
Contributor Author

Is it ok to run gtest only when testing Java? You could then do linkDebug and get *.so file without running gtests. Is this the intention?
...

For me, it is more idiomatic to have tests run in the test step. I found it confusing to have always test that run at every build.
We can still call it explicitly with gtest and they are linked in the testDebug steps.

Though I agree this is a matter of taste.

@r1viollet r1viollet force-pushed the r1viollet/remove_gtest_during_build branch from 964e71f to 82afa1e Compare July 24, 2025 13:34
@jbachorik
Copy link
Collaborator

Though I agree this is a matter of taste.

It's mostly about whether we feel it is ok to be able get the library binary without going through the gtest by default.
I have no strong opinion in this case - as long as we run the gtests eventually :)

@r1viollet
Copy link
Contributor Author

I had issues with file ownership on some alpine builds. I'm not sure why I was the only one to see this.

@r1viollet r1viollet requested a review from jbachorik July 28, 2025 08:06
@r1viollet r1viollet merged commit 38a5ddd into main Jul 28, 2025
95 checks passed
@r1viollet r1viollet deleted the r1viollet/remove_gtest_during_build branch July 28, 2025 12:24
@github-actions github-actions bot added this to the 1.29.0 milestone Jul 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants