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

Migrate to jakarta namespace #238

Merged
merged 3 commits into from Jun 20, 2023
Merged

Migrate to jakarta namespace #238

merged 3 commits into from Jun 20, 2023

Conversation

reda-alaoui
Copy link
Contributor

@reda-alaoui reda-alaoui commented Jan 22, 2023

Fixes #237
I don't know what to do with httpunit. Any advice? :)

@jetztgradnet
Copy link

We are currently migrating an application using this proxy servlet to JakartaEE and are looking for a solution as well.
@reda-alaoui I will test your draft PR #238 and provide some feedback.
@dsmiley what would work for you regarding a timeline of a release, assuming the PR is ready and working?

@stuplarosa
Copy link

We are also waiting for this change so that we can move to Tomcat 10.

@jetztgradnet
Copy link

@reda-alaoui regarding httpunit:

I found an updated version at hazendaz/httpunit which works with JakartaEE.

So instead of

    <dependency>
      <groupId>httpunit</groupId>
      <artifactId>httpunit</artifactId>
      <version>1.7</version>
      <scope>test</scope>
    </dependency>

use this:

    <dependency>
      <groupId>com.github.hazendaz.httpunit</groupId>
      <artifactId>httpunit</artifactId>
      <version>2.0.0</version>
      <scope>test</scope>
    </dependency>

@jetztgradnet
Copy link

I tested this locally and for me this worked. And the tests also run successfully with the updated httpunit variant mentioned in the previous comment.

@jetztgradnet
Copy link

@reda-alaoui may I suggest to update http-unit to the aforementioned version and publish this draft PR?

pom.xml Show resolved Hide resolved
@EdgewareRoad
Copy link
Contributor

EdgewareRoad commented Feb 26, 2023 via email

@dsmiley
Copy link
Collaborator

dsmiley commented Feb 26, 2023

I'll branch the current master branch to maybe "branch_1x" leaving the current master branch for the new 2x.

@EdgewareRoad
Copy link
Contributor

EdgewareRoad commented Feb 26, 2023 via email

@jetztgradnet
Copy link

There are different ways to go about this. In paultuckey/urlrewritefilter#242 instead of migrating everything they use a Maven plugin to create a different -jakarta variant of the library by doing bytecode rewriting after the main build.

But IMHO in the long run, having a 2.0 release for the Jakarta-based servlet containers should be the right path.

@dsmiley
Copy link
Collaborator

dsmiley commented Feb 27, 2023

That's a fantastic idea @jetztgradnet ! Basically use shading and a Maven classifier to ship it. I didn't know it could be so easy. This allows me to have it both ways -- support javax.servlet and also jakarta.servlet at the same time! One branch of changes instead of porting to another branch or abandoning a branch and thus a subset of the user community. Of course it won't be tenable forever; there may eventually be changes specific to the new Jakarta version but there has been none yet.

I'd like to go in that direction. LMK if there are concerns.

@reda-alaoui
Copy link
Contributor Author

@dsmiley as you can see at https://github.com/mitre/HTTP-Proxy-Servlet/pull/238/files#r1101714206 , this is not a simple package replacement.

@jetztgradnet
Copy link

@reda-alaoui right, but adding the shading plus changing this line is still possible, right? the setStatus(int) method has been there al the time, it's only the variant with the additional message line that was remove.

@jetztgradnet
Copy link

@dsmiley whether with a new major version for jakrata-only or a dual-variant approach, it would be great to get a release version soon :-)

@reda-alaoui
Copy link
Contributor Author

@reda-alaoui right, but adding the shading plus changing this line is still possible, right? the setStatus(int) method has been there al the time, it's only the variant with the additional message line that was remove.

I agree for this particular case. But more cases like that may appear in the near future.

More generally, I think allowing people to stay for years on javax is harmful to the ecosystem. So if you want to do it like that, fine, but I'll let someone else do it :)

@jetztgradnet
Copy link

jetztgradnet commented Feb 27, 2023

So we now have two possible approaches:

  1. provide two variants of the library, the main one unchanged and a variant with classifier jakarta, as a 1.13 release
  2. leave the 1.x branch unchanged and migrate the main branch to Jakarta EE with 2.0 release providing only this way, i.e. essentially this PR provided by @reda-alaoui

@dsmiley so which way do you want to go? I am following @reda-alaoui argument, but at the same time it is my main short-term interest in getting a release supporting Jakarta EE with any of the two approaches:

If you prefer the first approach, I would be willing to provide a PR with that one-line change required to work for Servlets 6.x plus the shading (assuming we can simply transfer the approach from paultuckey/urlrewritefilter#242).

@dsmiley
Copy link
Collaborator

dsmiley commented Feb 27, 2023

Then we do a bit of both. Embrace the "shade" mechanism to allow one maintained codeline and release but also do everything this PR is doing (new major version; use Jakarta in the maintained code). The "shade" configuration would be to produce a "javax-servlet" classifier (or something like that; dunno if there's a standard).

@jetztgradnet
Copy link

That sounds like a great compromise. In the simplest case we can take the example from the urlrewritefilter and reverse 'javax.servlet' and 'jakarta.servlet'.

@reda-alaoui would you also do that or would you want me to provide a snippet for that?

@dsmiley any preference for a classifier name? 'javax' or 'javax.servlet'

@dsmiley
Copy link
Collaborator

dsmiley commented Feb 27, 2023

I guess javax would be more generalized as the overall matter isn't just servlets. It's shorter too.

@jetztgradnet
Copy link

jetztgradnet commented Feb 28, 2023

So I didn't want to re-create @reda-alaoui's PR or provide a different PR include that commit, but adding this snippet to pom.xml after line 155 produces both jars when building the project using maven source:jar install:

      <plugin>
        <groupId>org.apache.maven.plugins</groupId>
        <artifactId>maven-shade-plugin</artifactId>
        <version>3.2.2</version>
        <executions>
          <execution>
            <id>javax</id>
            <phase>package</phase>
            <goals>
              <goal>shade</goal>
            </goals>
            <configuration>
              <shadedClassifierName>javax</shadedClassifierName>
              <shadedArtifactAttached>true</shadedArtifactAttached>
              <createDependencyReducedPom>false</createDependencyReducedPom>
              <artifactSet>
                  <includes>
                    <include>${project.groupId}:${project.artifactId}</include>
                  </includes>
              </artifactSet>
              <relocations>
                <relocation>
                  <pattern>jakarta.servlet</pattern>
                  <shadedPattern>javax.servlet</shadedPattern>
                </relocation>
                <relocation>
                  <pattern>jakarta.annotation</pattern>
                  <shadedPattern>javax.annotation</shadedPattern>
                </relocation>
              </relocations>
            </configuration>
          </execution>
        </executions>
      </plugin>

Verifying generated jars:

> find /home/myuser/.m2/repository/org/mitre
/home/myuser/.m2/repository/org/mitre/
/home/myuser/.m2/repository/org/mitre//dsmiley
/home/myuser/.m2/repository/org/mitre//dsmiley/httpproxy
/home/myuser/.m2/repository/org/mitre//dsmiley/httpproxy/smiley-http-proxy-servlet
/home/myuser/.m2/repository/org/mitre//dsmiley/httpproxy/smiley-http-proxy-servlet/2.0-SNAPSHOT
/home/myuser/.m2/repository/org/mitre//dsmiley/httpproxy/smiley-http-proxy-servlet/2.0-SNAPSHOT/smiley-http-proxy-servlet-2.0-SNAPSHOT-sources.jar
/home/myuser/.m2/repository/org/mitre//dsmiley/httpproxy/smiley-http-proxy-servlet/2.0-SNAPSHOT/smiley-http-proxy-servlet-2.0-SNAPSHOT-javax.jar
/home/myuser/.m2/repository/org/mitre//dsmiley/httpproxy/smiley-http-proxy-servlet/2.0-SNAPSHOT/smiley-http-proxy-servlet-2.0-SNAPSHOT.jar
/home/myuser/.m2/repository/org/mitre//dsmiley/httpproxy/smiley-http-proxy-servlet/2.0-SNAPSHOT/_remote.repositories
/home/myuser/.m2/repository/org/mitre//dsmiley/httpproxy/smiley-http-proxy-servlet/2.0-SNAPSHOT/maven-metadata-local.xml
/home/myuser/.m2/repository/org/mitre//dsmiley/httpproxy/smiley-http-proxy-servlet/2.0-SNAPSHOT/smiley-http-proxy-servlet-2.0-SNAPSHOT.pom
/home/myuser/.m2/repository/org/mitre//dsmiley/httpproxy/smiley-http-proxy-servlet/maven-metadata-local.xml

So @reda-alaoui or @dsmiley, whoever is first, would either of you add this commit to this PR or afterwards after merging this PR?
And hopefully @dsmiley would then release a binarty artifact on Maven Central (after possibly adding a paragraph to the README pointing out support for both versions)? ;-)

@dsmiley
Copy link
Collaborator

dsmiley commented Feb 28, 2023

sure thing. I'll do a release this weekend

@reda-alaoui reda-alaoui force-pushed the jakarta branch 2 times, most recently from eb99a83 to c7de0d7 Compare March 1, 2023 09:46
@reda-alaoui
Copy link
Contributor Author

@jetztgradnet @dsmiley I added the shading plugin

fxprunayre added a commit to fxprunayre/core-geonetwork that referenced this pull request Jun 14, 2023
 Password stored in database / EncryptedStringType not supported by jasypt jasypt/jasypt#147
 Cache / Ehcache 2 to 3 migration https://www.ehcache.org/documentation/3.10/migration-guide.html and https://docs.jboss.org/hibernate/orm/6.2/userguide/html_single/Hibernate_User_Guide.html#caching-provider-jcache-cache-manager
 Java / ReadOnlyMvcInterceptor / Something to remove?
 Java / ServiceManager / EofException
 Java / MailUtil / use jakarta.mail.*?
 Spring security / Check openid (getClientRegistration), keycloak (todo), cas (change to apero)
 Wro4J / No support for Spring6 wro4j/wro4j#1135
 HTTP Proxy / Jakarta migration in progress mitre/HTTP-Proxy-Servlet#238 (comment)
 Camel / https://camel.apache.org/blog/2022/05/camel317-whatsnew/.
@jetztgradnet
Copy link

@plan3d are you going to remove those old httpclient version or would you prefer if somebody else jumps in? I didn't want to just push another commit to this PR without checking with you first...

@jetztgradnet
Copy link

I assume removig support/testing for old httpclient versions would mean to simply remove those lines from .github/workflows/maven.yml?

@jetztgradnet
Copy link

jetztgradnet commented Jun 14, 2023

Or maybe this would rather be for @reda-alaoui to answer: would you simply remove the tests for older httpclient versions? Or is it ok if somebody else adds another commit to your PR?

@jetztgradnet
Copy link

@reda-alaoui any comment from your side on removing the old httpclient tests?
Would be great to get this finally closed and released... :-)

@reda-alaoui
Copy link
Contributor Author

On it.

@reda-alaoui reda-alaoui marked this pull request as draft June 17, 2023 15:42
@reda-alaoui reda-alaoui marked this pull request as ready for review June 17, 2023 15:43
@reda-alaoui
Copy link
Contributor Author

Done. Looks like someone has to trigger the build.

@jetztgradnet
Copy link

@reda-alaoui thanks for providing this update!
@dsmiley would you mind re-triggering the build?
And if successful, possibly already merge and release this change? Thanks!

Copy link
Collaborator

@dsmiley dsmiley left a comment

Choose a reason for hiding this comment

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

Thanks everyone!

The CHANGES.md needs to be updated as does README.md. Can that be done please? README references older HttpClient support which is now false. Needn't be thorough but don't want wrong information. README should reference the dual classifier thing.

@reda-alaoui
Copy link
Contributor Author

@dsmiley CHANGES.md and README.md updated.

@reda-alaoui reda-alaoui requested a review from dsmiley June 20, 2023 06:50
@dsmiley
Copy link
Collaborator

dsmiley commented Jun 20, 2023

Please don't force-push changes to PRs. It means reviewers (like me) can't see what changes you did between now and before.

CHANGES.md Outdated Show resolved Hide resolved
@reda-alaoui
Copy link
Contributor Author

reda-alaoui commented Jun 20, 2023

Please don't force-push changes to PRs. It means reviewers (like me) can't see what changes you did between now and before.

Ok noted. Some repositories want 1 commit per PR. Others don't. That's why I prefer Gerrit over Github.

Co-authored-by: David Smiley <dsmiley@apache.org>
@reda-alaoui reda-alaoui requested a review from dsmiley June 20, 2023 20:33
@dsmiley
Copy link
Collaborator

dsmiley commented Jun 20, 2023

Interesting... never seen such a 1-commit per PR policy. In GitHub, it's trivial to view all changes collapsed -- it's the default view if you click the "Files Changed" view. So I don't get the point.

README.md Outdated Show resolved Hide resolved
Co-authored-by: David Smiley <dsmiley@apache.org>
@reda-alaoui reda-alaoui requested a review from dsmiley June 20, 2023 20:44
@dsmiley dsmiley merged commit 43e7680 into mitre:master Jun 20, 2023
3 checks passed
@reda-alaoui reda-alaoui deleted the jakarta branch June 20, 2023 21:51
@dsmiley
Copy link
Collaborator

dsmiley commented Oct 4, 2023

I noticed Jakarta Servlet spec 5.0 was introduced here but that there are newer specs like 6 (judging from a question in #242 ) that was raised today. Is there an opinion here for/against raising the spec level, and to what?

@reda-alaoui
Copy link
Contributor Author

@dsmiley , I think my comment still stands.

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.

Spring boot 3 incompatibility
8 participants