atom feed17 messages in org.apache.myfaces.devRe: [DISCUSS] how to get rid of tons ...
FromSent OnAttachments
Mark StrubergOct 22, 2011 4:08 pm.txt
Jakob KorherrOct 23, 2011 2:59 am 
Mark StrubergOct 23, 2011 8:55 am 
Jakob KorherrOct 23, 2011 9:14 am 
Leonardo UribeOct 23, 2011 11:27 am 
Leonardo UribeOct 23, 2011 12:08 pm 
Mark StrubergOct 23, 2011 3:37 pm 
Leonardo UribeOct 23, 2011 7:35 pm 
Mark StrubergOct 24, 2011 1:37 am 
Leonardo UribeOct 24, 2011 10:17 am 
Mark StrubergOct 24, 2011 3:14 pm 
Leonardo UribeOct 24, 2011 3:29 pm 
Leonardo UribeOct 24, 2011 7:02 pm 
Mark StrubergOct 24, 2011 11:38 pm 
Mark StrubergOct 25, 2011 4:32 am 
Leonardo UribeOct 25, 2011 8:47 am 
Mark StrubergOct 25, 2011 9:23 am 
Subject:Re: [DISCUSS] how to get rid of tons of duplicated code
From:Mark Struberg (stru@yahoo.de)
Date:Oct 24, 2011 11:38:14 pm
List:org.apache.myfaces.dev

Leo, that's fine, but be aware that I'll go on unifying the codestyle over the
next days. So please be aware that you most likely will need to merge this branch manually.

LieGrue, strub

----- Original Message -----

From: Leonardo Uribe <lu4@gmail.com> To: MyFaces Development <de@myfaces.apache.org>; Mark Struberg
<stru@yahoo.de> Cc: Sent: Tuesday, October 25, 2011 12:30 AM Subject: Re: [DISCUSS] how to get rid of tons of duplicated code

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 <stru@yahoo.de>:

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 <lu4@gmail.com> To: MyFaces Development <de@myfaces.apache.org>; Mark Struberg <stru@yahoo.de> 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 <stru@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 <lu4@gmail.com>  To: MyFaces Development <de@myfaces.apache.org>; Mark

Struberg

<stru@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 <stru@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 <lu4@gmail.com>   To: MyFaces Development <de@myfaces.apache.org>   Cc: Mark Struberg <stru@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 <lu4@gmail.com>:

   Hi

   2011/10/23 Jakob Korherr

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

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