Re: [DISCUSS] how to get rid of tons of duplicated code - Leonardo Uribe - org.apache.myfaces.dev

17 messages in org.apache.myfaces.devRe: [DISCUSS] how to get rid of tons ...

From	Sent On	Attachments
Mark Struberg	Oct 22, 2011 4:08 pm	.txt
Jakob Korherr	Oct 23, 2011 2:59 am
Mark Struberg	Oct 23, 2011 8:55 am
Jakob Korherr	Oct 23, 2011 9:14 am
Leonardo Uribe	Oct 23, 2011 11:27 am
Leonardo Uribe	Oct 23, 2011 12:08 pm
Mark Struberg	Oct 23, 2011 3:37 pm
Leonardo Uribe	Oct 23, 2011 7:35 pm
Mark Struberg	Oct 24, 2011 1:37 am
Leonardo Uribe	Oct 24, 2011 10:17 am
Mark Struberg	Oct 24, 2011 3:14 pm
Leonardo Uribe	Oct 24, 2011 3:29 pm
Leonardo Uribe	Oct 24, 2011 7:02 pm
Mark Struberg	Oct 24, 2011 11:38 pm
Mark Struberg	Oct 25, 2011 4:32 am
Leonardo Uribe	Oct 25, 2011 8:47 am
Mark Struberg	Oct 25, 2011 9:23 am

Subject:	Re: [DISCUSS] how to get rid of tons of duplicated code
From:	Leonardo Uribe (lu4...@gmail.com)
Date:	Oct 24, 2011 10:17:37 am
List:	org.apache.myfaces.dev

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,

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

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

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

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

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

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

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

	Start a set with this search
	Include this search in one of my sets
	Exclude this search from one of my sets