-
Notifications
You must be signed in to change notification settings - Fork 269
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
FREEMARKER-218 - Add jakarta.servlet support #94
Conversation
That's a lots of unrelated changes in a single PR, which will make it very difficult (like slow) to merge. Admittedly, that's also because I have very limited time for FM recently, but that what we have. Given that some of these changes are backward compatible, it would be good the break this up, be done with this set of changes piece by piece. The other thing is, the new Jakarta API can be supported next to the old, so it need not be BC breaking, and things can stay a 2.3.x then. Supporting multiple Java versions was done through the whole history of the project (different java files are compiled with different versions), so that's a not a problem either. Surely it adds some complexity, but also removes the BC obstacle (i.e., if we go for 2.4, there's the logical desire to pack in more changes than supporting Jakarta).
|
Also, unlike with 3.0, 2.4 would use the same packages, so... it's not possible to have both 2.3 and 2.4 in the classpath (except for OSGi). So backward compatibility breaking can be painful for some users. (Also, we probably don't want to support 2.3.x and a 2.4.x line, and less backward compatibility breaking helps there too, like, politically.) So, yeah, keeping BC is possible as far as I know, and it would make the process much easier, as far as I see. Even if, yes, the source code is then more complex... this is a tradeoff. |
Also, there's an old PR that migrates 2.3.x from Ant/Ivy to Gradle. That should be merged first, and then some of these changes are not applicable anymore. I know, why it's the for so long there unmerged... well, I put that on the top of my FM pipeline, and, hoping it will be done in week or so. |
Thanks for the feedback, @ddekany I'm not sure I understand how you'd like these changes to be incrementally merged.
I would agree with you about BC once the project uses a "modern" modularised build system such as Gradle, which would allow us to publish two sets of JARs, one with javax and one with jakarta. You have a point about the versioning, I agree these changes make more sense as part of release 3. I've been a massive supporter of FreeMarker for almost two decades but, having spent the last week trying to perform what I thought would be a very straightforward upgrade, I must admit that keeping BC at all costs is harming this project. Branch Many of the artifacts relying on it on mvnrepository.com seem to be packaging it as part of web or email frameworks, without necessarily extending FM capabilities. (admittedly, I haven't checked all of them) From my own experience too, I've never decided to use the FreeMarker JAR because it had that one class that would be so useful on my project. Instead, its value lies in its capabilities. I'll keep this PR open (I'll adjust the version number) and will start packaging and distributing my fork in the meantime, as this will unlock a smoother upgrade to Spring 6 for many projects. I wanted to add a big thank you for your great work on this project. As mentioned, I've been using it for many years and always thought it was awesome, so please see the above as constructive feedback rather than criticism. I really value all the hard work that has gone into FreeMarker. |
FreeMarker always supported multiple version of Java, and multiple version of the same library (especially multiple version of the servlet API) in the same single jar. So you can add Jakarta support without removing anything, and breaking anything. Gradle doesn't change any of that (though with Gradle FM 2 is at least modularized on source code level). The only reason FreeMarker 2 is not modularized on the artifact level (unlike 3) is backward compatibility, and that Maven dependency management is to simplistic to handle such a change. BC is a huge pain, yes. But users just add it as a single simple dependency, write templates, and don't care why it's working behind the scenes, and I want allow fearless updates. But, most BC pain actually comes from issues in the template language itself, which can't be fixed without heavily breaking backward compatibility with existing templates. That's what 3 is really about (would be, if I invest the time...), although along the way it also fixes lot of historical mistakes in the API. (And 3 lives in different packages, it's not a problem if some dependency of your project uses 2, and some other uses 3.) I'm not sure what's up with Spring 6 support. Is Spring MVC FreeMarker support depend on Servlet API? Because JSP 2 "Model 2" is really old school at this point. (Besides, FreeMarker support was always a second class check box feature in Spring. First because the official choice was JSP, and then because of Thymeleaf. The only way to have proper Spring support would be to put the resources on that on the FreeMarker side.) |
Actually... the other pending PR-s will enter merge conflict status if I merge the Gradle PR... So, forget that, as that will happen when I managed to reduce the set of pending PR-s to just that. Meanwhile, my question is if you are willing rework this to be a non-breaking change. As you might seen in the Ant file, we compile different set of classes with different dependencies (including different java version). And then, on runtime we use Java reflection to decide which version of a support class to use. Since these things (Servlet API, Jython API, etc.) are provided dependencies as far as we are concerned, we just have to "magically" work in the random environment we are dropped into. |
I fully agree about BC for FreeMarker features, there has never been any issue with that part. I can do some of the adjustments. About Spring support, I'm not sure I understand what you mean. It has always been very straightforward to configure and use FreeMarker view resolvers, including in combination with other tools such as Tiles. About dependency management, I won't do that part until Gradle has been put in place and different dependencies can be managed properly via modules. Abusing optional dependencies is a bad practice and makes things harder for users, especially on large projects. |
BC and the pending gradle PR are the reasons I just opened the jira issue without providing a PR myself. Why spending hours of adding a feature if everything needs to be re-done when the gradle switch was finally made? Since there are tools like https://github.com/nebula-plugins/gradle-jakartaee-migration-plugin it's not a blocker anyway. |
Again, the Gradle PR is irrelevant in this. It will not change anything in this regard in the 2.x branch, as the binary output has to be the same as it was with Ant: a single artifact. All the dependencies of that are optional. But we can't even declare them like that in the Maven POM, as it doesn't have the concept of having optional dependencies of multiple versions of the the same thing. So, just do it with Ant, it's just copy-paste-modify of what we already do. |
Not sure if this is the way you wanted it and even if it works. It's not tested yet. |
@dtrunk90 Yes, I guess something like #95 will work. @ppodgorsek Sorry about this. It's better to discuss more involved changes on the dev list before investing into them. |
That's ok. Looking at the code of versions 2.x/3.x and the discussion we had here were insightful. |
Our codebase is dependent on FreemarkerServlet this PR seems a good solution. What are the plans for the Jakarta upgrade? |
@ppodgorsek I have created a temporary fork of freemarker based on your PR. We are gonna use that one internally until we have an actual Jakarta compatible release of freemarker. Thanks 🙏 |
Unfortunately, we will have to duplicate everything servlet-related, into its own package. Otherwise we break apps that expect the original Servlet API. So, if someone wants to work on a PR, that's what it should do. We can't just change the existing classes. |
That's not true. With Maven we can create an additional artifact with a transformer which just replaces the imports. That's probably true for Gradle as well. |
From the perspective of the JVM, you end up having a |
@firatkucuk As you can see in the above discussion, a few points were raised:
It's unlikely FreeMarker 3 will be production-ready when/if it is released, as the unclean git history will very likely introduce regressions and bugs. Added to the fact that projects still relying on Probably not what you wanted to read, but just my two cents. |
3 is irrelevant here. Treat is as vapor ware until it produced some candidate. |
Well, some provide 2 artifacts (1 for javax, 1 for Jakarta; commons-fileupload2 for example) and others are going with the future and just force their users to migrate to Jakarta EE (spring 6 for example). But for the time being I'm just using the Tomcat Migration tool to patch incompatible dependencies. So, for this discussion I'm out. It's up to the project maintainers to decide which way they want to go. I totally understand the pain as this simple namespace change has one of the biggest impacts in the entire Java history. Edit: you could probably check thymeleaf how they did it. Afaik they support both with the same artifact. |
Yeah, actually, there's the othe way of "duplicating", where we have freemarker:freemarker:2.3.33, and freemarker:freemarker:2.3.33-jakarta artifas. Kind of like Guava does it. Or use Maven classifiers. Had to research which have what gotchas in practice. |
So, we have to decide which of the end results we want. Those are, multiple artifact with different classifier (or version), or separate freemarker.servlet and freemarker.jakartaservlet packages? Or, third, just break stuff, and require users to migrate to Jakarta. Then we can decide how to achieve that end result. (And I will be surprised if the current ancient Ant build can't do it. I mean, yes, we do migrate away from Ant, but let's not act as if that's a blocker, until we know it.) |
Maybe you could also just integrate the Migration tool into the ant build process and let it do the work for you. It's just a java class to call which will create a 2nd patched artifact: https://github.com/apache/tomcat-jakartaee-migration Not sure exactly how and if it's possible for a project itself. But it definitely works if you have freemarker as a dependency (or jar file) in your project for example. |
That's exactly what I suggested. No new in packages. Maven transformers do exactly the same thing. |
Well, during the December sprint (latest) I will probably start trying https://github.com/apache/tomcat-jakartaee-migration, as that even works with the current Ant solution. Hoping it will be done in a few hours, that's surely worth it. (There are many other things that users are waiting for, and a few weeks a year to grind on them, so... you know, priorities.) Of course, PR-s can help a lot too, as far as they try to keep a practically small scope. |
Update: Mailing list discussion is here: https://lists.apache.org/thread/s15x9gzyhpr9d7cj8ck6kzgz3l6nfjrf So, we have to choose which end result we want (ignore the "how" for now):
Possibility 1 has the pro that we don't have to publish one more artifact, and also, then users don't have to fiddle with dependency management to chose the "jakarta" classifier artifact. But it has the con, that any existing dependent Java code that used Actually, I will bring it up on the mailing list... if you are also there, please answer there. |
Hello, any news about this? I see the mailing list discussion hasn't been very active lately.... |
The mail discussion was in favor of adding a new package in the monolithic artifact, if we count the answers. So probably that will be it. But we do this after the other PR-s were settled, ending with the Gradle one, as the solution to this will (certainly) involve the build tool. BTW, I have checked what's with Spring Web MVC integration. At a point (StringBoot 3) they have switched to Jakarta, and has simply removed some of the Servlet/JSP related features from the Spring FreeMarker integration (and I don't think many will miss them). So, whatever we do, Spring integration is not affected. |
What features of Servlet/JSP support are you using? I'm asking because support JSP taglibs (TLD-based) might brings some complications, if that has to work on modern containers. |
Yes, we do use JSP Taglibs in this manner
|
Pushed the change from PR #104. At least based on the JUnit tests, So, now the freemarker jar artifact contains |
Awesome, thank you. |
It's in 2.3.33, which I guess (no promises) will be released late February. You can help with testing/using this new feature, and trying to break it. |
Why not increase the version to 2.4.0? |
That's just how it always worked at this project... 2.4.0 is reserved for minor backward compatibility breaks. 2.3.x-es are 2.3.0 compatible (except that minimum dependency requirements can increase, in a conservative way). To clarify, this version still support legacy Servlet/JSP as well, so it's backward compatible. Yeah, maybe in the future we should switch to semver... |
Note that this is included in 2.3.33-SNAPSHOT that's uploaded to Apache Maven snapshot repo: https://repository.apache.org/content/groups/snapshots/org/freemarker/freemarker/2.3.33-SNAPSHOT/
|
Hi, any updates on the actual release date for the jakarta support? |
That would be the release date of 2.3.33. It's waiting for more testing and the voting at this point, no blocker missing features/fixes. So few more weeks, I think. But, please test it if you can. You don't even have to build it, as it's in the Apache snapshot repo (see above). |
FYI we've been using/testing it for a few weeks now and we've had no issues |
I will prioritise its testing |
Just to be clear (for anyone reading), what you are testing is actually the other Jakarta PR, #104. That's what's in the 2.3.33-SNAPSHOT build that I have linked earlier, not this PR where we are now commenting. |
Hi, any updates on the actual release date of 2.3.33? |
The Apache OFBiz project will finalise its test period next week : https://issues.apache.org/jira/browse/OFBIZ-12935 |
We have integrated 2.3.33 into our codebase and facing no issues so far. didn't have comprehensive testing yet though. |
Same here |
when will version 2.3.33 be released? |
Well, last time it was late February... yeah. My current guess is that we can start voting this week, and then the voting passes in 1-2 weeks. |
FYI, voting on 2.3.33 release has started yesterday: Let's hope it passes... |
don`t work, 2.3.33 or 2.3.34-SNAPSHOT |
You have to use freemarker.ext.jakarta.servlet package instead of freemarker.ext.servlet package, and freemarker.ext.jakarta.jsp instead of freemarker.ext.jsp, everywhere in your application. It's not enough to just update your FreeMarker dependency. We support both pre-Jakarata and Jakarta environments in the same artifact, so the name of the non-Jakarta classes did not change. Also note that 2.3.33 contains https://github.com/apache/freemarker/pull/104, and not this PR. |
What has been changed in this PR:
Given the scope of the changes, I would suggest we create a
2.4-gae
branch and change the target of this PR before merging it. All files and documentation in this PR already refer to these changes as being part of release 2.4.I tried performing this change on branch
3
but it is incredibly far from2.3-gae
's head. Furthermore, the git history of branch3
seems to be completely messed up, some gradle files are linked to old Java files, other files have been renamed without it being tracked. To avoid regressions, I would advise to rename3
to3-deprecated
, to create a new3
branch from2.4
, and to cherry-pick new changes as needed.