Hi Daniel, Thanks for the answers! Please see my comments inline.
On Tue, Nov 17, 2015 at 4:28 PM, Daniel Dekany <[email protected]> wrote: > Tuesday, November 17, 2015, 10:07:17 PM, Woonsan Ko wrote: > >> Hi Daniel, >> >> I'm sorry for late response. Have been busy with something else.. > > Maybe it was just an oversight, but let's keep the discussion on the > developer list on the future. Oh, I guess I just clicked on Reply on your message where the dev e-mail address was CC'ed. Sorry for this. :-) > >> On Mon, Nov 9, 2015 at 5:02 PM, Daniel Dekany <[email protected]> wrote: >>> Tuesday, November 3, 2015, 8:31:41 AM, Woonsan Ko wrote: >>> >>>> Hi, >>>> >>>> I've just made an initial experiment PR to use Maven instead: >>>> - https://github.com/apache/incubator-freemarker/pull/8 >>>> >>>> Build works well mostly and seems to create an artifact properly >>>> (target/freemarker-2.3.24-SNAPSHOT.jar). >>>> >>>> However, as described in the PR, there are some issues and things to note: >>>> - Two unit tests fail: RealServletContainertTest#basicTrivial() and >>>> RealServletContainertTest#basicELFunctions(). >>>> This seems related to war initialization issue. For example, the >>>> existing ant build copies all the necessary jar files into WEB-INF/lib >>>> somehow, but the new maven doesn't have that. >>> >>> I haven't looked into this (yet), but there's no Ant magic involved. >>> After all the JUnit tests run successfully be merely right clicking on >>> the project in Eclipse, then choosing Run as JUnit Test. >> >> I might have been misled by "macrodef name="buildExampleWebapp" (which >> copies and compiles some .java files in >> src/examples/@{exampleName}/WEB-INF/classes) in build.xml, but it >> seems unrelated to the normal unit testing. > > It's unrelated. Those are just some legacy example "projects". If they > cause problems, we can just remove them. Evangelia has started to > write some tutorials on GitHub, and something like that is certainly a > better medium for this purpose nowadays. OK. Thanks for the info. > >> It might be also related to my changes using jsp-25 api only which >> seems incompatible with the old jetty engine while unit testing. >> I'll try to look into it further. >> >>> >>>> - To support incompatible multiple versions of jython, I decided to >>>> use the latest version of jython only and use Reflection APIs for the >>>> older versions of jython in two classes because those two classes are >>>> very small/simple and seems more effective using Reflection APIs. >>> >>> I'm worried about runtime efficiency there, at least when it comes to >>> getPythonClassName. Reflection is relatively slow, especially a lot of >>> it. >> >> OK, I see. I'll try with different approaches. >> >>> >>>> - Similarly, I simply added a new jsp-api 2.1 operation in a base >>>> class to support both 2.0 and 2.1 at runtime. I think this approach >>>> should be simpler than complex dependency management and multiple >>>> compilations. >>> >>> You are still linking to JSP 2.1 classes there (JspApplicationContext >>> and ELContext as the declared return value), even if you are just >>> throwing UnsupportedOperationException there instead of returning an >>> instance. And so it will still fail on runtime, unless I miss >>> something. >> >> Hmm, really? The jsp api jar is provided by the container, so I don't >> think there's a problem unless we explicitly use a new API in jsp-2.1 >> when running in container supporting only jsp-2.0 api. > > AFAIR, you have JSP 2.1 classes (like JspApplicationContext) in the > method signatures of the classes that are meant to be used un a JSP > 2.0 container. Ah, right. That's definitely incompatible changes at runtime in 2.0 container. Thanks for the pointer! I searched on the internet to understand what JspFactory is for and found this one in the online doc: "even when run in an environment that doesn't have its own JSP implementation, the FreeMarker JSP runtime will make available its own implementation of JspFactory and JspEngineInfo to tags when JSP 2.0 API JAR is available in classpath, as well as an implementation of JspApplicationContext when JSP 2.1 API JAR is available in class path." [1] So, are those classes mainly for supporting JSP tags in non-container-provided-JSP environment? [1] http://freemarker.incubator.apache.org/docs/versions_2_3_11.html > >>>> Regarding RealServletContainertTest and testing with embedded jetty >>>> server, one of natural solutions in Maven is to organize the project >>>> into multiple modules. >>>> For example, maybe we can change the project structure into this: >>>> >>>> - freemarker >>>> |- core >>>> `- test >>>> `- web >>>> >>>> If test/web is a 'war' package project, then we don't have to copy >>>> jars into WEB-INF/lib manually, but we can probably take advantage of >>>> the existing features of jetty plugin or other (e.g, cargo) for >>>> packaging in (integration)test phase, for instance. >>> >>> Right now I hope that we actually don't do any such hack with Ant >>> either, and so there won't be Maven work around either. (See earlier.) >> >> Yeah, but there are many javac tasks with different classpaths for >> different sets of .java files, which can be interpreted in Maven to >> multiple projects, each of which has one set of dependencies. Maybe >> there are better solutions in Maven, but that's what I meant. > > What I'm meant is that the JUnit tests use a single unified > "classpath", no tricks. Otherwise they wouldn't run from Eclipse > either. Though that classpath mostly contains the higher version of > everything, which isn't wise to use for compilation. But then, Maven > does support having different dependencies for the tests. OK. Different classpath sets in compile mode, but one classpath set (with the latest deps) in test mode today. Good to know that. Thank you very much for your answers with the details. I'll try to address those issues mentioned above somehow. Kind regards, Woonsan > >>>> Also, the Reflection API usages for those two test classes seem okay >>>> enough to me, but if there's any strong reason to validate those as >>>> early as possible, then maybe we can add more submodules for each >>>> specific purpose. >>> >>> I don't see reflection in *test* classes in the pull request. You >>> meant the non-test classes? >> >> Ah, sorry. I meant _Jython20And21VersionAdapter.java and >> _Jython22VersionAdapter.java, not test classes. Sorry for my >> confusion. >> >>> >>>> Anyway, I'd like to share my initial experiment with you folks and >>>> hear from you about approaches. >>>> If we need to restructure the modules, then it should be better to get >>>> a consensus in prior anyway. >>> >>> So, the reason I though Maven won't cut it is exactly the problem with >>> linking to multiple dependency versions. Right now the number of such >>> cases is quite low, compared to most earlier FreeMarker versions, but >>> I don't know how that will change in the future. Anyway, I would >>> accept any solution where it's just awkward to do these, but using a >>> solution that substantially affects the actual output (i.e., has >>> runtime impact), all because of the build tool, well... it's hard to >>> accept for me. OTOH I do want Maven, and ready to accept all kind of >>> horrid hacks as far as they won't affect the runtime. And so I'm >>> thinking if we could just drop in some classes as resources, and those >>> classes are just built manually and dropped and committed into the >>> source... very very awkward, yes, but for the few cases we will endure >>> it, and then the runtime won't be affected (no reflection used). Only, >>> then some special test cases will be needed to check if those classes >>> aren't out of sync with what they meant to link to and such. >> >> OK. Thanks for your remarks. >> I'll try to look into it further and let you know if I have something >> better. :-) >> >> Cheers, >> >> Woonsan >> >>> >>> (BTW, we will also need the OSGi meta data, Manual building, etc. But >>> all that when everything else is done.) >>> >>> -- >>> Thanks, >>> Daniel Dekany >>> >>> >>>> Kind regards, >>>> >>>> Woonsan >>>> >>>> >>>> ---------- Forwarded message ---------- >>>> From: Daniel Dekany <[email protected]> >>>> Date: Sat, Oct 31, 2015 at 1:02 AM >>>> Subject: Re: Question on unit testing >>>> To: Woonsan Ko <[email protected]> >>>> >>>> >>>> It would be in fact very good news if this thing can go with Maven... >>>> looking forward for the results! >>>> >>>> >>>> Thursday, October 29, 2015, 10:54:50 AM, Woonsan Ko wrote: >>>> >>>>>>> By the way, IIRC, you mentioned in old ML that it's planned to migrate >>>>>>> to Maven some day. Is it right? If so, when do you want to do that? >>>>>> >>>>>> I'm not sure if that would work out well, given that FM needs to be >>>>>> compiled against multiple versions of the same library (including >>>>>> rt.jar). But if someone works this topic out, it might turns out that >>>>>> it's feasible after all. I don't know. But the bet is on Gradle. And >>>>>> that will probably only happen when someone other than me takes that >>>>>> task. >>>>> >>>>> I think it should be possible with Maven (e.g, by using multiple >>>>> activatable profiles and system dependencies). I'd like to experiment >>>>> it in a branch. Will let you know if I make some progress. :-) >>>>> >>>>> Cheers, >>>>> >>>>> Woonsan >>>>> >>>> >>>> -- >>>> Thanks, >>>> Daniel Dekany >>>> >>> >> > > -- > Thanks, > Daniel Dekany >
