myfaces-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Leonardo Uribe <lu4...@gmail.com>
Subject Re: [DISCUSS] how to get rid of tons of duplicated code
Date Mon, 24 Oct 2011 17:18:00 GMT
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 <struberg@yahoo.de>:
>>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 <lu4242@gmail.com>
>> To: MyFaces Development <dev@myfaces.apache.org>; Mark Struberg <struberg@yahoo.de>
>> 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 <struberg@yahoo.de>:
>>>  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 <lu4242@gmail.com>
>>>>  To: MyFaces Development <dev@myfaces.apache.org>
>>>>  Cc: Mark Struberg <struberg@yahoo.de>
>>>>  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 <lu4242@gmail.com>:
>>>>>   Hi
>>>>>
>>>>>   2011/10/23 Jakob Korherr <jakob.korherr@gmail.com>:
>>>>>>   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 <struberg@yahoo.de>:
>>>>>>>   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
>>>>>>
>>>>>
>>>>
>>>
>>
>

Mime
View raw message