Hi I started a branch to try it:
http://svn.apache.org/repos/asf/myfaces/core/branches/2.0.x_refactor_shade_duplicate/ just to gain some time. I think it is worth at least to try a prototype. regards, Leonardo Uribe 2011/10/24 Mark Struberg <[email protected]>: > 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 >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
