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

[FREEMARKER-7] convert to Maven build #96

Closed
wants to merge 1 commit into from

Conversation

bmarwell
Copy link
Contributor

@bmarwell bmarwell commented Oct 26, 2023

fixes FREEMARKER-7

// Edit: I got tricked by JIRA and mistakenly thought that the issue was still open. Sorry for the confusion.
// There is of course no need to add Maven to a working ANT and Gradle build (branches 2.3-gae and 3 respectively).


This is a PoC for a proposal:
Move the current 3-Branch to 4, and make a new 3.0.0-branch with only the following features:

  • Maven Build (this PR) fixes FREEMARKER-7
  • Bump to Java 8 (this PR)
  • Remove old Jython, JSP and servlet implementations (this PR)
  • Add module-info.java (later PR)
  • relocate base package to org.apache.freemarker (currently: freemarker).

This would be a very slight change for users which would be a good stepping stone towards a 4.0 release (current 3.x branch).

@bmarwell bmarwell force-pushed the 2.3-gae-maven branch 2 times, most recently from a3395e7 to 0a84190 Compare October 26, 2023 08:29
@bmarwell
Copy link
Contributor Author

[ERROR] Tests run: 899, Failures: 12, Errors: 0, Skipped: 2

I'd say not too bad for the first try :)

@bmarwell
Copy link
Contributor Author

Down to three test errors:

Error:  Failures: 
Error:    RealServletContainertTest.basicELFunctions:116->WebAppTestCase.assertJSPAndFTLOutputEquals:150->WebAppTestCase.assertOutputsEqual:155->WebAppTestCase.getResponseContent:92 Expected HTTP status 200, but got 500 (message: Server Error) for URI http://localhost:36263/basic/tester?view=customELFunctions1.jsp
Error:    RealServletContainertTest.basicELFunctionsTagNameClash:124->WebAppTestCase.assertJSPAndFTLOutputEquals:150->WebAppTestCase.assertOutputsEqual:155->WebAppTestCase.getResponseContent:92 Expected HTTP status 200, but got 500 (message: Server Error) for URI http://localhost:36263/basic/tester?view=elFunctionsTagNameClash.jsp
Error:    RealServletContainertTest.basicTrivial:77->WebAppTestCase.assertJSPAndFTLOutputEquals:150->WebAppTestCase.assertOutputsEqual:155->WebAppTestCase.getResponseContent:92 Expected HTTP status 200, but got 500 (message: Server Error) for URI http://localhost:36263/basic/tester?view=trivial.jsp

Sadly there is not description for the exception:

Error:  Tests run: 17, Failures: 3, Errors: 0, Skipped: 1, Time elapsed: 3.509 s <<< FAILURE! -- in freemarker.ext.jsp.RealServletContainertTest
Error:  freemarker.ext.jsp.RealServletContainertTest.basicELFunctions -- Time elapsed: 0.040 s <<< FAILURE!
java.lang.AssertionError: Expected HTTP status 200, but got 500 (message: Server Error) for URI http://localhost:36263/basic/tester?view=customELFunctions1.jsp
	at org.junit.Assert.fail(Assert.java:89)
	at freemarker.test.servlet.WebAppTestCase.getResponseContent(WebAppTestCase.java:92)
	at freemarker.test.servlet.WebAppTestCase.assertOutputsEqual(WebAppTestCase.java:155)
	at freemarker.test.servlet.WebAppTestCase.assertJSPAndFTLOutputEquals(WebAppTestCase.java:150)
	at freemarker.ext.jsp.RealServletContainertTest.basicELFunctions(RealServletContainertTest.java:116)
	at java.lang.reflect.Method.invoke(Method.java:498)

Error:  freemarker.ext.jsp.RealServletContainertTest.basicTrivial -- Time elapsed: 0.020 s <<< FAILURE!
java.lang.AssertionError: Expected HTTP status 200, but got 500 (message: Server Error) for URI http://localhost:36263/basic/tester?view=trivial.jsp
	at org.junit.Assert.fail(Assert.java:89)
	at freemarker.test.servlet.WebAppTestCase.getResponseContent(WebAppTestCase.java:92)
	at freemarker.test.servlet.WebAppTestCase.assertOutputsEqual(WebAppTestCase.java:155)
	at freemarker.test.servlet.WebAppTestCase.assertJSPAndFTLOutputEquals(WebAppTestCase.java:150)
	at freemarker.ext.jsp.RealServletContainertTest.basicTrivial(RealServletContainertTest.java:77)
	at java.lang.reflect.Method.invoke(Method.java:498)

Error:  freemarker.ext.jsp.RealServletContainertTest.basicELFunctionsTagNameClash -- Time elapsed: 0.015 s <<< FAILURE!
java.lang.AssertionError: Expected HTTP status 200, but got 500 (message: Server Error) for URI http://localhost:36263/basic/tester?view=elFunctionsTagNameClash.jsp
	at org.junit.Assert.fail(Assert.java:89)
	at freemarker.test.servlet.WebAppTestCase.getResponseContent(WebAppTestCase.java:92)
	at freemarker.test.servlet.WebAppTestCase.assertOutputsEqual(WebAppTestCase.java:155)
	at freemarker.test.servlet.WebAppTestCase.assertJSPAndFTLOutputEquals(WebAppTestCase.java:150)
	at freemarker.ext.jsp.RealServletContainertTest.basicELFunctionsTagNameClash(RealServletContainertTest.java:124)
	at java.lang.reflect.Method.invoke(Method.java:498)

@ddekany
Copy link
Contributor

ddekany commented Oct 27, 2023

As of migrating away from Ant, there's already a PR for a Gradle-based solution, that keeps binary backward compatibility, while still modularizes the source code (as it's in the current 3.x). That also can handle the cases where we depend on multiple versions of the same artifact (like even of Java), which we sometimes need (in 2.x, as we have a monolithic artifact, for backward compatibility).

As of the Maven package name change, I'm against changes that are annoyances for the users without bringing significant new benefits in exchange. At least last time I checked, renaming a Maven group is a pain for its consumers, because then Maven won't do version conflict resolution between the old and new artifacts, and you end up having multiple versions of the same classes in your application. (So then you should also rename the Java packages, so the two artifacts can co-exist, but that's an even more drastic change, and should have really good reasons. Like when you want to break backward compatibility in template language rules anyway, as in the current 3.x, then you should do that. But not just for aesthetic reasons for sure.)

@bmarwell
Copy link
Contributor Author

As of migrating away from Ant, there's already a PR for a Gradle-based solution, that keeps binary backward compatibility, while still modularizes the source code (as it's in the current 3.x). That also can handle the cases where we depend on multiple versions of the same artifact (like even of Java), which we sometimes need (in 2.x, as we have a monolithic artifact, for backward compatibility).

I know. But since we are Apache, you'd get plenty of help from other foundation members, committers etc. We already have an Apache-based project setup which adds the benefit of streamlined releases, reproducible releases etc.
If you want to do the same work again, I won't stop you. But that means I cannot contribute, as my knowledge of Gradle is very limited. And that might be true for a lot of other Apache committers.

As for the backward compatibility:
Breaking changes is what major releases are for. It it was not breaking anything, don't update the major version.

As of the Maven package name change, I'm against changes that are annoyances for the users without bringing significant new benefits in exchange.

No need to do that right now, BUT it would be worth it to streamline Apache projects.
Again, if you need help -- the maven team can help you.

At least last time I checked, renaming a Maven group is a pain for its consumers, because then Maven won't do version conflict resolution between the old and new artifacts, and you end up having multiple versions of the same classes in your application.

That is not entirely true. First of all, dependabot, renovate etc. can read the relocation from the pom.xml:
https://maven.apache.org/guides/mini/guide-relocation.html

(So then you should also rename the Java packages, so the two artifacts can co-exist, but that's an even more drastic change, and should have really good reasons. Like when you want to break backward compatibility in template language rules anyway, as in the current 3.x, then you should do that. But not just for aesthetic reasons for sure.)

Other projects did this as well, see Junit 4 to JUnit Jupiter. I never saw anyone being trapped with this. On the contrary, it was helpful that two major versions could co-exist: Migration does not need to be done at the same time for all files. This could be true for Freemarker.


To summarize:

With only the changes mentioned in the first post of the PR, users would have a migration release which does not expect a lot of work:

  • stay at single-jar dependency
  • relocation is pretty easy
  • renaming packages is pretty easy
  • module identifier is consistent, not split packages
  • Path to Java 8 opened
  • Abandon old JavaEE 5 APIs.

Again, I am only saying "move your 3 branch to 4 and keep Gradle".
I did not say "abandon it".

The idea is that I want to use freemarker with JPMS right now and don't want to wait for the current-3-branch for two more years with so many breaking changes etc. which is way more pain.

@bmarwell
Copy link
Contributor Author

As for backwards compatible binary: We can still do that in a multi-module setup with maven. And I disagree with your opinion that this is less feasible with Maven. We could do that right now.

Copy link
Member

@hboutemy hboutemy left a comment

Choose a reason for hiding this comment

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

this PR looks reasonably simple, but it still changes things that are not necessary (groupId, JDK runtime prerequisite)
even the removal of 4 classes si questionable: is it necessary to do that now? Or do it in a future different PR?

@@ -33,7 +33,7 @@
<ivy pattern="${localMaveRepoDir}/[organisation]/[module]/[revision]/[module]-[revision].pom" />
</filesystem>
-->
<ibiblio name="mavenCentral" m2compatible="true" />
<ibiblio name="mavenCentral" m2compatible="true" root="https://nexus.intern/repository/central/" />
Copy link
Member

@hboutemy hboutemy Nov 2, 2023

Choose a reason for hiding this comment

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

what is this root="https://nexus.intern/repository/central/"? it feels like a personal setup: is it really expected to be shared?

<version>30</version>
</parent>

<groupId>org.apache.freemarker</groupId>
Copy link
Member

Choose a reason for hiding this comment

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

why change the groupId? it's not necessary nor useful for this step
please focus this PR to the minimal changes: one PR per topic



<properties>
<javaVersion>8</javaVersion>
Copy link
Member

Choose a reason for hiding this comment

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

is it absolutely necessary top switch to Java 8 in this PR?
again, this PR should have minimal impact on the output vs previous build: changing prerequisite JDK is a future topic

@bmarwell
Copy link
Contributor Author

bmarwell commented Nov 2, 2023

The idea is that this is merged into a new branch, a new 3 branch. Yes, the idea here is to change a few things, see on the mailing list.

Intermediate release with just

Java 8
Remove some old APIs (jsp, servlet).
...

But yes, changing the location is not needed here.

And the removal of the duplicate classes could be done in another pr, but would need to come first

@ddekany
Copy link
Contributor

ddekany commented Nov 2, 2023

To clarify, for anyone else looking at this, FREEMARKER-7 was closed like 5 years ago. That's when the "3" branch switched to Gradle. Then even that was modernized, and there's a similar Gradle PR for 2.3.x: #79 (FREEMARKER-204). I'm not aware of any issue with that.

@hboutemy
Copy link
Member

hboutemy commented Nov 3, 2023

there's a similar Gradle PR for 2.3.x: #79 (FREEMARKER-204). I'm not aware of any issue with that.

oh, I did not know: the only issue is that it's not merged

@bmarwell
Copy link
Contributor Author

bmarwell commented Nov 4, 2023

Thanks for the clarification. Closed in favour of new PRs.

@bmarwell bmarwell closed this Nov 4, 2023
@ddekany
Copy link
Contributor

ddekany commented Nov 4, 2023

there's a similar Gradle PR for 2.3.x: #79 (FREEMARKER-204). I'm not aware of any issue with that.

oh, I did not know: the only issue is that it's not merged

Yeah... The reason it's not merged that it makes the source code modular, just like in "3" (but here it still generates a single monolithic jar), so if I merge it, I will break all pending PR-s (or I think so). So I have to review and merge those first... And I couldn't catch up so far. And then merge the Gradle one. And then have time to check if it can go into the release.

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.

3 participants