Hi Leo! Yes, this sounds much better, but please give me 2 days to think through it in detail.
txs and LieGrue, strub ----- Original Message ----- > From: Leonardo Uribe <[email protected]> > To: MyFaces Development <[email protected]>; Mark Struberg > <[email protected]> > Cc: > Sent: Monday, October 24, 2011 7:18 PM > Subject: Re: [DISCUSS] how to get rid of tons of duplicated code > > Hi > > Ok, it is evident we have different opinions here about how to deal > with this problem. So, rather than refute the arguments I think it is > more productive to show another proposal. In MyFaces Core 2.0.x we > have the following layout: > > api/ > assembly/ > bundle/ > impl/ > implee6/ > integration-tests/ > pom.xml > shared/ > src/ > > Let's add two modules > > parent : contains the parent POM that all submodules should inherit. > shared-utils : utilities for JSF used in core, but built as an api. > > Now, we move the duplicate code related to myfaces-commons-utils into > this module, and shared will have shared-utils as dependency. impl > module will use maven-shade plugin to take the code from shared-utils > and shared (actually this is done for only for shared). > > When a release of myfaces-core is done, all modules are released, so > things go as usual. But have the parent as module has the advantage > that if we want to release shared-utils or shared internal modules, we > can do it only releasing parent (optional) and shared-utils. > > Since we can create a release for these modules, we remove the hack > used on on shared project (hard copy). Just we modify the pom to use > maven-shade-plugin over myfaces-core shared module. Then we kill > shared-tomahawk, and we make tomahawk use maven-shade-plugin over > shaded artifact in shared project. > > In commons we do the same as in shared. In > myfaces-commons-resourcehandler we use myfaces core shared module > using maven-shade-plugin. > > The only disadvantage is the release process gets a little bit more > complicated, but since do a release becomes easier with nexus, it is > ok. > > Do you agree with this solution? > > regards, > > Leonardo Uribe > > 2011/10/24 Mark Struberg <[email protected]>: >>> Really that was not solved using maven-shade-plugin. >> Then we used the maven-shade-plugin the wrong way. See the > <relocations> option [1]. >> >>> There, there is a profile called >>> "synch-myfaces-impl-shared", when it is added, the code is > copied and >>> then a manual commit do the trick. >> >> I think this is an ugly hack and doesn't solve any problems because >> >> a.) >>> Take into account >>> that each release requires a vote and that vote takes 3 days to get >>> fixed. >> you could just do a mvn release of shared + core in 1 go to the same > staging repo -> only 1 vote is needed! >> This argument is simply wrong. >> >> b.) >> You >> currently copy the code over 1:1 (half manually) thus your argument >> with 'core and other projects need different sources' is just nil. > There >> is no difference if you do this by profile, by hand or automatically! >> So I really prefer to have this automatically. Which is exactly what a >> dependency does... >> >> c.) the TCK argument is moot because it has >> nothing to do with shared. Most of the issues in the TCK are not >> affecting shared. And if they do, then those fixes are needed BY ALL >> OTHER PROJECTS AS WELL. Thus another argument against hiding this code >> and duplicating it all over... >> >> c.) >>> Instead, maybe the option is reorganize myfaces core to allow >>> alternate release lifecycles per module >> Sorry I don't grok this argument. It sounds as it would make all things > more complicated without solving any real problem. >> >> e.) >>> This means myfaces-commons project should be "merged" in some > way with >>> myfaces core. It has sense. >> 2 options: >> 1..) kill myfaces-shared completely and use the shared from core instead. >> Downside: if you need some fix in the shared code for some other project, > you would need to release mf-core >> 2.) kill the newly introduced (this got only created a few weeks back by > you) core-shared and use mf-shared again. >> Downside: hmmm nothing if one understands how to release correctly. >> >> f.) >> all your explanations only explain the duplication between >> myfaces-shared and myfaces-core-shared. I can live with the duplication >> for now, but we also have classes which got copied around up to 8 times! >> There is no excuse for that imo. >> >> g.) >>> But what happen when you have some code that does not have a clear >>> "interface". If somebody removes or change some code because > he/she >>> thinks it is not used in core or whatever, all 6 projects that could >>> require it will be affected and will require to rework its code. >>> Things get uglier when you have one library working with version 1.1.1 >>> and 1.1.2 is binary incompatible with version 1.1.1, but my other >>> dependency requires it and kaboooom, the application does not work. >>> So, the first assumption we need to preserve in those > "shared" >>> artifacts is build it as an API, preserving binary compatibility. >> >> I don't get that argument neither! >> Hey, >> that's life! If it turns out that the code is not good enough and > needs >> a fix, then that's the way it is! All other projects should fix that >> too in that case. I rather have a reproducible compile error which >> easily could get fixed than having tons of duplicated code which is more >> or less always logically broken and badly tested. >> Yes, we should be >> aware that the classes we put into myfaces-shared must meet some >> standards and need to be well tested. But actually that would benefit >> our project a lot. >> >> >> h.) I just realised that our process in copying shared-impl from core to > mf-shared is even more broken than every process before. >> If you are working on a lets say mf-commons project and find a bug in any > of those shared parts, then you would need to RELEASE MF-CORE FIRST? omg, > this > cannot be serious! >> >> >> LieGrue, >> strub >> >> >> [1] > http://maven.apache.org/plugins/maven-shade-plugin/shade-mojo.html#relocations >> >> >> >> ----- Original Message ----- >>> From: Leonardo Uribe <[email protected]> >>> To: MyFaces Development <[email protected]>; Mark Struberg > <[email protected]> >>> Cc: >>> Sent: Monday, October 24, 2011 4:36 AM >>> Subject: Re: [DISCUSS] how to get rid of tons of duplicated code >>> >>> Hi >>> >>> 2011/10/23 Mark Struberg <[email protected]>: >>>> I've now read through the old mail archives and understand > what the >>> original problem was. But actually I don't think we solved it > correctly >>> right now. Of course we solved to original problem, but opened a can of > worms >>> causing other problems. >>>> >>>> The problem as far as I remember has been that myfaces-shared had > tons of >>> duplicated code in it. One for core, one for tomahawk, one for > trinidad, etc. >>>> >>>> >>>> The shared part for core got moved to myfaces-core, but the deeper > problem >>> was that it was not easily possible to have multiple different versions > of >>> myfaces-shared. This now got solved by using the maven-shade-plugin. So > we >>> should rethink the practice to duplicate all the code and aim for a > _clean_ >>> solution. >>>> >>> >>> Really that was not solved using maven-shade-plugin. What we did was >>> copy the code into myfaces-core and create a mirror of the same code >>> under shared. There, there is a profile called >>> "synch-myfaces-impl-shared", when it is added, the code is > copied and >>> then a manual commit do the trick. >>> >>>> Also (being a maven guy) I cannot quite follow the argument about > the >>> release cycles. Running a myfaces-shared release and then (with the > same staging >>> repo) a myfaces-core release is a task of 15 minutes. + the time for > running the >>> TCK, but this gets run via CI anyway, right? Thus this is barely a > problem. >>>> If it is then I'd happily volunteer to do the next release (do > this for >>> a few projects already) As you know, performing a release really got > _much_ >>> easier nowadays with our new apache-parent pom. >>>> But maybe this argument was only meant for our old release process > (which I >>> agree was a lot of work)? >>>> >>>> If your answer is 'it's still needed' then can we just > unify >>> all other usages? >>>> >>> >>> Make a release is just the first of the problems. Take into account >>> that each release requires a vote and that vote takes 3 days to get >>> fixed. So in practice a problem in core can effectively block a >>> release of other artifacts. That's very inconvenient. Suppose we > have >>> a new TCK and that one found a problem on myfaces core. Again even if >>> the other artifacts are good enough, this becomes a blocker. There are >>> enough historical evidence that supports this point. In conclusion >>> this slow down the whole release cycle we have on myfaces. So ignore >>> that is not an option. >>> >>> Instead, maybe the option is reorganize myfaces core to allow >>> alternate release lifecycles per module. For example, each maven >>> plugin in myfaces has its own release lifecycle and there is a parent >>> pom with a different release procedure. This requires some changes to >>> create the source-release.zip file inherited from apache pom. But it >>> could be a cleaner solution. >>> >>> This means myfaces-commons project should be "merged" in some > way with >>> myfaces core. It has sense. >>> >>>> One question which bothers me with the 'shared' approach > if what >>> would happen to our build-tools annotation scanning > (@JSFWebConfigParam, etc)? >>> Does this already work with dependencies? Do we have this problem > already due to >>> the fact that we import such annotated classes via dependency? >>>> >>> >>> Those annotations comes from myfaces-builder-annotations. They are >>> source code annotations but all that information are saved on >>> myfaces-metadata.xml, so even if dissapear on compile time, the >>> information can be gathered from there. It is not a problem. >>> >>>>> Additionally, we increase the risk of "side > effects", >>>>> because a change done in core could introduce a bug in other > parts. >>>> Imo it's exactly the opposite. If you use the same code in 7 > projects, >>> then it is more likely that a bug gets found and fixed. >>>> And the opposite case is (sadly) absolutely unlikely. If you have > a class >>> duplicated 7 times and find a bug in one project, it is highly unlikely > that all >>> 6 other projects will get this fix applied :( >>>> >>> >>> But what happen when you have some code that does not have a clear >>> "interface". If somebody removes or change some code because > he/she >>> thinks it is not used in core or whatever, all 6 projects that could >>> require it will be affected and will require to rework its code. >>> Things get uglier when you have one library working with version 1.1.1 >>> and 1.1.2 is binary incompatible with version 1.1.1, but my other >>> dependency requires it and kaboooom, the application does not work. >>> So, the first assumption we need to preserve in those > "shared" >>> artifacts is build it as an API, preserving binary compatibility. >>> >>> So we can't just grab the code from shared as is and say to users > "... >>> you can use that into its own projects ...". If the project is >>> maintained inside myfaces we can fix such problems, but outside >>> myfaces we should be more strict. So, we need a "public > shared" code >>> like the one proposed in myfaces commons and other code "myfaces >>> shared" to use in projects like tomahawk or portletbridge or > whatever >>> inside our land. >>> >>> regards, >>> >>> Leonardo Uribe >>> >>>> LieGrue, >>>> strub >>>> >>>> >>>> >>>> ----- Original Message ----- >>>>> From: Leonardo Uribe <[email protected]> >>>>> To: MyFaces Development <[email protected]> >>>>> Cc: Mark Struberg <[email protected]> >>>>> Sent: Sunday, October 23, 2011 9:08 PM >>>>> Subject: Re: [DISCUSS] how to get rid of tons of duplicated > code >>>>> >>>>> Hi >>>>> >>>>> Ok, let's check the proposal >>>>> >>>>> MS>> So the suggestion is: >>>>> MS>> >>>>> MS>> 1.) cleanup myfaces-shared. mf-shared has almost no >>> checkstyle >>>>> rules applied. >>>>> >>>>> Yes, sounds good. >>>>> >>>>> MS>> 2.) add unit tests for myfaces-shared. Currently > there are >>> not >>>>> many... >>>>> >>>>> Ok, sounds good too. >>>>> >>>>> MS>> 3.) move the shared code parts back to > myfaces-shared and >>> add unit >>>>> tests. >>>>> >>>>> So, this means do one step back and move the code from > myfaces-core >>>>> "shared" to myfaces-shared project? This breaks > effectively >>> the >>>>> changes done some months ago to make easier work with myfaces > core >>>>> itself. >>>>> >>>>> In that time the conclusion was: "core has priority over > anything >>>>> else, so shared code must live in core, but myfaces-shared > project >>>>> should just copy the code from there and have its own > lifecycle" >>>>> (these are my own words as I understood). >>>>> >>>>> So this point does not have practical sense, and go against > everything >>>>> discussed earlier. >>>>> >>>>> MS>> 4.) import myfaces-shared via maven dependency and > use >>>>> <minimizeJar> and <relocations> to package the > stuff >>>>> >>>>> maven-shade-plugin is a good "tool" but doesn't > fit well >>> in this >>>>> scenario. The reason is we need an alternate release lifecycle > for the >>>>> shared code between myfaces core and other projects. >>>>> >>>>> Historically that was the very first intention behind > myfaces-shared >>>>> project. Any myfaces core release requires some additional > steps to do >>>>> (TCK), so that becomes a problem when you try to release other >>>>> libraries that depends of shared. So, to fix that, > "shared" >>> was >>>>> created, so the code can be released in a independent way, and > prevent >>>>> myfaces core becomes an obstacle to release any other project >>>>> (tomahawk, portlet-bridge, ... ). So, to release tomahawk you > release >>>>> shared first and then tomahawk. >>>>> >>>>> maven-shade-plugin requires a released artifact to do its job. > So, use >>>>> it impose that restriction. In "shared" case, > preserve the >>> original >>>>> intention becomes "imperative", and that's the > reason why >>> a goal >>>>> was >>>>> created to copy the code from myfaces-core shared, so the > release >>>>> manager can run this goal, commit the changes and then run a > release. >>>>> >>>>> My proposal in this case is do the same we did for shared, but > for >>>>> "myfaces commons" case. Then we can use > maven-shade-plugin in >>> other >>>>> projects, but not over shared, instead over a released version > of >>>>> myfaces-commons-utils. Keep tomahawk or portlet-bridge as is, > using >>>>> shared project, because by its nature, those projects require > classes >>>>> that are not meant to be used outside those cases. >>>>> >>>>> Note do any hack in this part makes a little bit > "obscure" >>> how to make >>>>> changes, because everything becomes "centralized", > but makes >>> easier >>>>> maintain code. Additionally, we increase the risk of > "side >>> effects", >>>>> because a change done in core could introduce a bug in other > parts. So >>>>> at the end this is a matter of how to keep our code >>> "balanced", even >>>>> if some times it becomes a decision about "choose the > less >>>>> inconvenient alternative". >>>>> >>>>> regards, >>>>> >>>>> Leonardo Uribe >>>>> >>>>> 2011/10/23 Leonardo Uribe <[email protected]>: >>>>>> Hi >>>>>> >>>>>> 2011/10/23 Jakob Korherr <[email protected]>: >>>>>>> Hi Mark, >>>>>>> >>>>>>> +1 - that's exactly what I have been trying to > accomplish >>> some time >>>>>>> ago (introducing common-shades [1]). Unfortunately, I > was not >>>>>>> successful back then. >>>>>>> >>>>>> >>>>>> It is clear we need to "split" myfaces-impl > into >>> multiple >>>>> modules. There >>>>>> are some parts that are useful for other projects. The > code you >>> did >>>>>> on commons-shade was the attempt to solve the problem of > the >>>>>> duplicate code used on myfaces-test. >>>>>> >>>>>> Now the objective is find a way about how to reuse code > in myfaces >>>>>> core between multiple projects effectively. >>>>>> >>>>>>> However, there is a slight problem with moving all > this stuff >>> into >>>>>>> MyFaces shared, which I want to point out: code size. > If we >>> really put >>>>>>> all the code that is shared across any MyFaces > subproject into >>> shared, >>>>>>> it will get fat and ugly (even more than it is right > now). In >>>>>>> addition, if we continue including the whole shared > project >>> into >>>>>>> MyFaces core, MyFaces core impl will get bigger and > bigger. >>>>>>> >>>>>> >>>>>> Yes, the problem basically is MyFaces shared does not > have any >>> order >>>>>> or any notion of API. There are code that is used only in > tomahawk >>> but not >>>>>> intended to use in any other place. There are some useful >>> utitlities but >>>>>> sometimes without documentation, and there are some other > code >>> that is >>>>>> just obsolete. It it clear a cleanup of that location is >>>>>> necessary, but note priorities comes first, so this task > has been >>> delayed >>>>> in >>>>>> order to deal with other important stuff. Now it is a > good time to >>> fix >>>>> this. >>>>>> >>>>>>> Thus I'd like to suggest something similar which > I wanted >>> to >>>>>>> accomplish with common-shades: Introduce a new shared > module, >>> which >>>>>>> consists of many submodules that each handle a > specific >>> functionality >>>>>>> instead of being one fat module. With this approach > each >>> MyFaces >>>>>>> subproject would be able to pick out only the stuff > it really >>> needs. >>>>>>> Furthermore we would see more easily which code in > shared is >>> not used >>>>>>> anymore (I guess at the moment there is a lot of it), > just by >>> checking >>>>>>> which modules are still used in our poms. >>>>>>> >>>>>> >>>>>> That is the big question, how to split myfaces-impl and > shared. >>> Precisely >>>>>> the intention of myfaces-commons-utils projects was take > the stuff >>> that is >>>>>> useful from shared and build an usable API for developers > outside >>> MyFaces. >>>>>> >>>>>> For example, MyFaces HTML5 subproject was a good > experiment to see >>>>>> which code is useful and should be added in a API. Some > weeks ago >>> I checked >>>>>> and removed all duplicate code to use > myfaces-commons-utils. So >>> the 1.0.2 >>>>>> release contains those classes taken from shared. >>>>>> >>>>>> regards, >>>>>> >>>>>> Leonardo Uribe >>>>>> >>>>>>> Regards, >>>>>>> Jakob >>>>>>> >>>>>>> [1] > https://svn.apache.org/repos/asf/myfaces/common-shades/ >>>>>>> >>>>>>> 2011/10/23 Mark Struberg <[email protected]>: >>>>>>>> Hi! >>>>>>>> While working on the mafyces-commons cleanup I > figured >>> that we have >>>>> tons of >>>>>>>> duplicated code spread over MyFaces. >>>>>>>> >>>>>>>> >>>>>>>> As an example I like to mention >>> myfaces-commons-resourcehandler. >>>>> There are >>>>>>>> 43 classes in total, and 35 of them are just 1:1 > copied >>> from other >>>>> projects >>>>>>>> to provide resource management, zip, etc. For me > this is >>> an >>>>> absolute no-go. >>>>>>>> Those classes have neither tests nor any > documentation >>> where they >>>>> got forked >>>>>>>> from. Nor will any bug which gets fixed in > another module >>> make >>>>> it's way over >>>>>>>> to all the other projects containing that very > forked >>> code. >>>>> That's just ... >>>>>>>> unbelievable unmaintainable. >>>>>>>> >>>>>>>> There are 2 different ways to solve this > (depending on the >>>>> problem): >>>>>>>> >>>>>>>> A.) drop the functionality and provide a > generalized >>> solution. The >>>>> GZIP of >>>>>>>> myfaces-commons-resourcehandleris an obvious > example: >>>>>>>> We now copy this code over the 4th time or even > more. >>> Instead of >>>>> doing this, >>>>>>>> we should rather do it in the classic unix > fashion: do one >>> thing, >>>>> but do it >>>>>>>> well. >>>>>>>> Which means I'd rather see all the GZIP stuff > factored >>> out into >>>>> an own >>>>>>>> mf-commons module as a Servlet Filter. This can > then get >>> applied to >>>>> what >>>>>>>> ever other mechanism you like. This could also > (commonly) >>> cover >>>>> cases like >>>>>>>> detecting http UserAgents which are not able to > handle >>> zipped >>>>> resources, >>>>>>>> etc. That way we could provide this logic ONCE > and have >>> complete >>>>> freedom >>>>>>>> over the configuration. >>>>>>>> >>>>>>>> B.) code reusable components once and use them in > other >>> projects >>>>> (ev via >>>>>>>> shading it in). >>>>>>>> ClassLoaderResourceLoader would be a perfect > candidate! I >>> grepped >>>>> through >>>>>>>> only the few pits which I have checked out > locally and >>> found this >>>>> class 7 >>>>>>>> SEVEN times! I just can't believe that we > can't >>> move this >>>>> stuff to a shared >>>>>>>> modul... >>>>>>>> >>>>>>>> Same for FacesServletMapping. 6 times copied > around, >>>>>>>> WebConfigProviderFactory 5 times, ... >>>>>>>> There are whole packages with 10++ classes which > got >>> copied 1:1! >>>>>>>> >>>>>>>> I really could cry seeing this :( >>>>>>>> >>>>>>>> >>>>>>>> What can we do to solve this? >>>>>>>> >>>>>>>> Theoretically myfaces-shared should contain this > stuff. >>> This is >>>>> exactly what >>>>>>>> it is for! >>>>>>>> Historically there have been some hand forged > tweeks and >>> ugly >>>>> hacks, but >>>>>>>> nowadays we have the maven-shade-plugin to make > our live >>> easier. >>>>>>>> >>>>>>>> So the suggestion is: >>>>>>>> >>>>>>>> 1.) cleanup myfaces-shared. mf-shared has almost > no >>> checkstyle >>>>> rules >>>>>>>> applied. >>>>>>>> 2.) add unit tests for myfaces-shared. Currently > there are >>> not >>>>> many... >>>>>>>> 3.) move the shared code parts back to > myfaces-shared and >>> add unit >>>>> tests. >>>>>>>> 4.) import myfaces-shared via maven dependency > and use >>>>> <minimizeJar> and >>>>>>>> <relocations> to package the stuff >>>>>>>> >>>>>>>> [+1] fine go ahead (ideally: yes, what parts can > I help >>> with?) >>>>>>>> [0] dont care >>>>>>>> [-1] wont work because ... >>>>>>>> >>>>>>>> >>>>>>>> I've attached a file which contains all > Classes which >>> name >>>>> exists multiple >>>>>>>> times in MyFaces. The number is the cound how > often they >>> exist in >>>>> MyFaces. I >>>>>>>> excluded current20. >>>>>>>> Please note that classes with the same name do > not >>> necessarily have >>>>> the same >>>>>>>> content - but quite a lot actually do have! > (scroll to the >>> bottom >>>>> of the >>>>>>>> file ...) >>>>>>>> >>>>>>>> LieGrue, >>>>>>>> strub >>>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> Jakob Korherr >>>>>>> >>>>>>> blog: http://www.jakobk.com >>>>>>> twitter: http://twitter.com/jakobkorherr >>>>>>> work: http://www.irian.at >>>>>>> >>>>>> >>>>> >>>> >>> >> >
