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

[gradle] Add generated protobuf headers to C++ test include path #5920

Closed
pjreiniger opened this issue Nov 12, 2023 · 5 comments · Fixed by #5926 or #5933
Closed

[gradle] Add generated protobuf headers to C++ test include path #5920

pjreiniger opened this issue Nov 12, 2023 · 5 comments · Fixed by #5926 or #5933

Comments

@pjreiniger
Copy link
Contributor

pjreiniger commented Nov 12, 2023

Is your feature request related to a problem? Please describe.
Currently generated protobuf headers are not put into the include path that is used to build the tests. I tried a couple things to fix add the include path but didn't make any progress and thought Thad might now how to fix it real quick. I found this when working on #5918, which is why that does not have C++ tests in the PR

To reproduce, add a test file like this into wpimath/src/test/native/cpp/geometry

#include <gtest/gtest.h>
#include "geometry.pb.h" // Gets a file not found error

TEST(IncludeTest, Test) {
    wpi::proto::ProtobufPose2d proto;
   proto.mutable_translation()->set_x_meters(5);
   std::cout << proto.DebugString() << std::endl;
}
@pjreiniger
Copy link
Contributor Author

pjreiniger commented Nov 16, 2023

I swore I had tried this and hoped you did it slightly differently than me, but this is what I was getting

https://github.com/wpilibsuite/allwpilib/actions/runs/6887451508/job/18734682127?pr=5918#step:6:3700

This should be reopened

pjreiniger added a commit to pjreiniger/allwpilib that referenced this issue Nov 16, 2023
@PeterJohnson PeterJohnson reopened this Nov 16, 2023
@KangarooKoala
Copy link
Contributor

The specific error message is this (lines 3699-3719 of the posted CI link):

Some problems were found with the configuration of task ':wpimath:compileWpimathTestReleaseGoogleTestExeWpimathTestCpp' (type 'CppCompile').
  - Gradle detected a problem with the following location: '/work/wpimath/build/generated/source/proto/main/cpp/geometry2d.pb.h'.
    
    Reason: Task ':wpimath:compileWpimathTestReleaseGoogleTestExeWpimathTestCpp' uses this output of task ':wpimath:generateProto' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed.
    
    Possible solutions:
      1. Declare task ':wpimath:generateProto' as an input of ':wpimath:compileWpimathTestReleaseGoogleTestExeWpimathTestCpp'.
      2. Declare an explicit dependency on ':wpimath:generateProto' from ':wpimath:compileWpimathTestReleaseGoogleTestExeWpimathTestCpp' using Task#dependsOn.
      3. Declare an explicit dependency on ':wpimath:generateProto' from ':wpimath:compileWpimathTestReleaseGoogleTestExeWpimathTestCpp' using Task#mustRunAfter.
    
    For more information, please refer to https://docs.gradle.org/8.4/userguide/validation_problems.html#implicit_dependency in the Gradle documentation.
  - Gradle detected a problem with the following location: '/work/wpimath/build/generated/source/proto/main/cpp/geometry3d.pb.h'.
    
    Reason: Task ':wpimath:compileWpimathTestReleaseGoogleTestExeWpimathTestCpp' uses this output of task ':wpimath:generateProto' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed.
    
    Possible solutions:
      1. Declare task ':wpimath:generateProto' as an input of ':wpimath:compileWpimathTestReleaseGoogleTestExeWpimathTestCpp'.
      2. Declare an explicit dependency on ':wpimath:generateProto' from ':wpimath:compileWpimathTestReleaseGoogleTestExeWpimathTestCpp' using Task#dependsOn.
      3. Declare an explicit dependency on ':wpimath:generateProto' from ':wpimath:compileWpimathTestReleaseGoogleTestExeWpimathTestCpp' using Task#mustRunAfter.
    
    For more information, please refer to https://docs.gradle.org/8.4/userguide/validation_problems.html#implicit_dependency in the Gradle documentation.

So I think the error is about Gradle configuration, not whether the protobuf headers are in the C++ include path.

@PeterJohnson
Copy link
Member

Ah, we need to add a dependency linkage... should be pretty straightforward to do that in shared/jni/setupBuild.gradle, but I'm not sure of the exact Gradle incantation to do it.

@KangarooKoala
Copy link
Contributor

Looks like we do https://github.com/wpilibsuite/allwpilib/blob/main/shared/jni/setupBuild.gradle#L71 for the compile base tasks:

            binaries.all {
                if (it instanceof SharedLibraryBinarySpec) {
                    it.buildable = false
                    return
                }
                it.cppCompiler.define 'WPILIB_EXPORTS'
                it.cCompiler.define 'WPILIB_EXPORTS'
                if (!project.hasProperty('noWpiutil')) {
                    lib project: ':wpiutil', library: 'wpiutil', linkage: 'shared'
                }
                it.tasks.withType(CppCompile) {
                    it.dependsOn generateProto // This is where we declare the dependency
                }
                if (project.hasProperty('splitSetup')) {
                    splitSetup(it)
                }
            }

@PeterJohnson
Copy link
Member

#5933 should fix that issue.

pjreiniger added a commit to pjreiniger/allwpilib that referenced this issue Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants