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

implement ability to skip generation of javax annotation #10927

Merged
merged 5 commits into from Feb 17, 2024

Conversation

alexanderankin
Copy link
Contributor

@alexanderankin alexanderankin commented Feb 16, 2024

supercedes #10786, improves #9179 situation

Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

It's a bit weird to name this "jakarta" since it isnt' Jakarta, but sure. Let's just remove the redundant case.

" comments = \"Source: $file_name$\")\n"
"@$GrpcGenerated$\n");

if (vars["JakartaMode"].compare("javax") == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Just delete this case, since it is the same as the final else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah its is totally wonky but i want to make it as dead simple as possible so it is obvious what the default behavior is. not a c++ guy so i am coding for the fellow non-c++ https://grugbrain.dev

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my prediction is you will have more disgruntled souls come in here because jakarta has replaced javax and that is what everyone is using these days. since spring boot has made the move, this accounts for like vast majority of people ¯_(ツ)_/¯.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so as consequence just structuring the code so it is obvious where to put the new code statements for jakarta

@alexanderankin
Copy link
Contributor Author

ok looks like it finally passes - pretty mergeable apart from the bit of grossness with the if else's: lmk if its a deal breaker

@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Feb 17, 2024
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Feb 17, 2024
@alexanderankin
Copy link
Contributor Author

Do I need to fix the kokoro build? I can try to fix that if it's replicatable locally

@larry-safran larry-safran merged commit 0d39c2c into grpc:master Feb 17, 2024
13 of 14 checks passed
@larry-safran
Copy link
Contributor

larry-safran commented Feb 17, 2024 via email

@ejona86
Copy link
Member

ejona86 commented Feb 20, 2024

@alexanderankin, it was a known issue that just cropped up. It is unrelated to your change. #10925

@paul8620
Copy link

When is there a release of this?

@larry-safran
Copy link
Contributor

This will be part of the 1.63.0 release which is targeted for early April.

@EduardoSemanas
Copy link

EduardoSemanas commented Apr 11, 2024

@alexanderankin @ejona86 @larry-safran could you help me implement the jakarta_mode omit? Where/How should I add it? I've tried through custom options on the .proto but with no luck there, maybe I'm understanding something wrong here

@panchenko
Copy link
Contributor

In gradle it looks like the following

protobuf {
    protoc { artifact = "com.google.protobuf:protoc:$Versions.protoc" }
    plugins { grpc { artifact = "io.grpc:protoc-gen-grpc-java:$Versions.grpc" } }
    generateProtoTasks {
        all()*.plugins { grpc { option 'jakarta_omit' } }
    }
}

@panchenko
Copy link
Contributor

The option name is not clear enough, so I have suggested #11086.

@EduardoSemanas
Copy link

EduardoSemanas commented Apr 11, 2024

@panchenko thank you so much. I'm using maven (probably should have mentioned it before) and having some trouble finding out how to specify generateProtoTasks with it. Do you have any idea how it should be done?

@EduardoSemanas
Copy link

UPDATE: I was able to implement it using maven like so:

<plugin>
    <groupId>org.xolstice.maven.plugins</groupId>
    <artifactId>protobuf-maven-plugin</artifactId>
    <version>0.6.1</version>
    <configuration>
        <protocArtifact>com.google.protobuf:protoc:3.12.0:exe:${os.detected.classifier}</protocArtifact>
        <pluginId>grpc-java</pluginId>
        <pluginArtifact>io.grpc:protoc-gen-grpc-java:${grpc.version}:exe:${os.detected.classifier}
        </pluginArtifact>
    </configuration>
    <executions>
        <execution>
            <configuration>
                <pluginParameter>
                    jakarta_omit
                </pluginParameter>
            </configuration>
            <goals>
                <goal>compile</goal>
                <goal>compile-custom</goal>
            </goals>
        </execution>
    </executions>
</plugin>

@alexanderankin
Copy link
Contributor Author

UPDATE: I was able to implement it using maven like so:

@EduardoSemanas - the option name has changed as per #11086 - if you want to update your example

--- before.xml	2024-04-19 09:46:24.169520648 -0400
+++ after.xml	2024-04-19 09:46:43.677325583 -0400
@@ -12,7 +12,7 @@
         <execution>
             <configuration>
                 <pluginParameter>
-                    jakarta_omit
+                    @generated=omit
                 </pluginParameter>
             </configuration>
             <goals>

@panchenko if you can update yours as well:

--- before.gradle	2024-04-19 09:48:05.884466658 -0400
+++ after.gradle	2024-04-19 09:48:19.112323278 -0400
@@ -2,6 +2,6 @@
     protoc { artifact = "com.google.protobuf:protoc:$Versions.protoc" }
     plugins { grpc { artifact = "io.grpc:protoc-gen-grpc-java:$Versions.grpc" } }
     generateProtoTasks {
-        all()*.plugins { grpc { option 'jakarta_omit' } }
+        all()*.plugins { grpc { option '@generated=omit' } }
     }
 }

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.

None yet

7 participants