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

introduce jakarta version of artemis-cdi-client #5479

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

Conversation

cortlepp
Copy link

This PR introduces artemis-cdi-jakarta-client, a Jakarta (i.e. Jakarta EE 9+) equivalent of artemis-cdi-client. The new module is based on artemis-cdi-client with all necessary changes to make it jakarta-compatible. No other changes were made. You can review the individual steps in the commit history.

I will also post about this on the dev mailing list and would appreciate any feedback.

@gemmellr
Copy link
Member

This looks like you manually copied the code to the new module and then changed it. None of the other -jakarta modules, such as artemis-jakarta-client which this relies on, were implemented that way with their own committed sources. Instead, the new module brings in the related original javax module code itself during the build and transforms it. That way, one set of code exists to update (or mostly not, in the case of artemis-cdi-client) and there is no need to synchronize changes or scope for them to diverge.

If creating this module then it should ideally be implemented the same way, especially as the original module is so rarely updated, mentioned, or seemingly used that discussing deleting even it came up last month just before you popped up asking about it (seemingly the first to do so).

- we have to remove the `@Override` annotation here because otherwise the compilation of artemis-cdi-jakarta-client will fail due to those methods no longer being a part of jakarta ee 9+
@cortlepp
Copy link
Author

Thanks for the feedback! I wasn't aware of how this transformation was done in the other modules.

As far as I can tell using the transformer plugin worked, with two exceptions.

  • the javax.enterprise.inject.spi.Extension file was just copied to the resources folder and not renamed to jakarta.enterprise.inject.spi.Extension. I added this file manually, thus both files are now in the resources of artemis-cdi-jakarta-client, which should not be problematic (I think).
  • the isNullable() method was removed from Bean in Jakarta ee9+, so we cannot override after transforming the source. I removed the @Override (from artemis-cdi-client), while it is unfortunate that we can't do this in a prettier way I also don't think this is a serious problem.

I have never worked with this transformer plugin before though, so it would be great if someone could take another look, maybe there is a better solution.

@gemmellr
Copy link
Member

I have never worked with the plugin either but had a dig and found that the plugin actually should process the ServiceLoader services file, and looking at it with debug logging it actually does do so and even indicates that the name should be updated, but then it just copies it with the original name.

Looking closer at how the plugin works, its really just some glue and it is the Eclipse Transformer doing the main work underneath. I had a look there and found the ServiceLoader processing, and looking at the changes to that file it looks like it specifically had a fix that seems like it is around this issue (eclipse-transformer/transformer@c2207ce) of not updating the file because it only needs renamed and has no change to the content.

Unfortunately, the plugin is a bit old and is using an older version of the transformer. Trying to have the build use newer versions with the fix above, it seems they aren't compatible.

Given it is just one file, I don't think its worth switching out to a different newer plugin just to get this working seamlessly. Your workaround of just adding the new services file directly seems fine for this specific case.

However I also don't think we should leave the incorrect services file around. Running an execution of the maven-clean-plugin during the generate-sources phase, just after the transform is done, to delete the copy is probably the simplest thing for now. E.g:

diff --git a/artemis-cdi-jakarta-client/pom.xml b/artemis-cdi-jakarta-client/pom.xml
index 218659cb69..f425880eec 100644
--- a/artemis-cdi-jakarta-client/pom.xml
+++ b/artemis-cdi-jakarta-client/pom.xml
@@ -157,6 +157,27 @@
                </dependency>
             </dependencies>
          </plugin>
+         <plugin>
+            <groupId>org.apache.maven.plugins</groupId>
+            <artifactId>maven-clean-plugin</artifactId>
+            <executions>
+               <execution>
+                  <id>remove-old-services-file</id>
+                  <phase>generate-sources</phase>
+                  <goals>
+                     <goal>clean</goal>
+                  </goals>
+                  <configuration>
+                    <filesets>
+                      <fileset>
+                        <directory>${project.build.directory}/generated-resources/transformed/META-INF/services/</directory>
+                      </fileset>
+                    </filesets>
+                    <excludeDefaultDirectories>true</excludeDefaultDirectories>
+                  </configuration>
+               </execution>
+            </executions>
+         </plugin>
       </plugins>
    </build>
 

@gemmellr
Copy link
Member

The new module should probably be named artemis-jakarta-cdi-client, given the convention of all the other similar such modules.

The new module should be added to the bom.

This change needs a Jira raised: https://issues.apache.org/jira/browse/ARTEMIS which should then be referenced in the PR title (see most other PRs except dependabot's).

The commits should be squashed, and the Jira referenced in the commit message (see existing commit history)

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.

2 participants