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 >>>>>> >>>>> >>>> >>> >> >
