myfaces-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Leonardo Uribe <lu4...@gmail.com>
Subject Re: Proposal: Get rid of shaded-impl module in core 2.1.x
Date Mon, 11 Jul 2011 22:01:24 GMT
Hi

2011/7/11 Gerhard Petracek <gerhard.petracek@gmail.com>:
> hi jakob,
> i suggest the same approach like before. last time leo requested some
> changes and had to start the vote (with a short description) and this time
> it's your turn.
> if you feel that we need a vote about it, please feel free to start one.

Of course, as long as the alternative proposed be possible and valid.
But anyway I think the discussion point (do not change packages) has
been accepted, right?.

Leonardo Uribe

> regards,
> gerhard
> http://www.irian.at
>
> Your JSF powerhouse -
> JSF Consulting, Development and
> Courses in English and German
>
> Professional Support for Apache MyFaces
>
>
>
> 2011/7/11 Jakob Korherr <jakob.korherr@gmail.com>
>>
>> Hi,
>>
>> Leo, I really get your point that we can't change this stuff. Changing
>> SPI stuff (even just renaming packages) will break application server
>> integation code, we all got it now..
>>
>> That's why I suggested (a few mails ago, but unfortunately too vague)
>> keeping the package names *.spi, *.spi.impl and *.config.element as
>> is, but still moving *.spi and *.config.element to a new module called
>> myfaces-spi. This alone will let us be able to remove shaded-impl, no
>> code change is actually required, just moving some classes from
>> myfaces-impl into myfaces-spi. In the end (--> the myfaces-impl jar),
>> it will all be back in myfaces-impl again, because of shade.
>>
>> I will provide a new patch by tomorrow and then start a vote for it.
>> There really is no technical reason for not committing such a solution
>> at this point.
>>
>> Regards,
>> Jakob
>>
>> 2011/7/11 Leonardo Uribe <lu4242@gmail.com>:
>> > Hi
>> >
>> > I'll be clear and direct. What makes statements true or false are the
>> > reasons behind it. Until this moment, you are not question the reasons
>> > behind the reasoning proposed.
>> >
>> > To be short, my argumentation is we can't change for now:
>> >
>> > 1. Everything inside org.apache.myfaces.spi
>> > 2. LifecycleProvided
>> > 3. Everything inside org.apache.myfaces.config.element
>> >
>> > because JSF 2.1 is a maintenance release (not a mayor release) which
>> > already has it first version (but even without a version). Do that
>> > will create bugs on server integration code. The problem is there is
>> > no way to detect such changes or create a proper patch from the server
>> > side point of view without use some ugly reflection code and that
>> > really s...cks!.
>> >
>> > Let it to JSF 2.2 (which is another JSR) sounds better because in that
>> > time I think we can get rid of implee6 too in one move!. In that
>> > version, just change the poms, and move all code to impl and that's
>> > it.
>> >
>> > In conclusion, here we have a technical restriction that doesn't allow
>> > us to move further, so we can't really make a vote because there is no
>> > decision to make, we just can't change the code!. The idea of an API
>> > in impl module is precisely make the "promise" that we will be nice
>> > and do not change that stuff until the next major version.
>> >
>> > Unfortunately there is nothing we can do in 2.1.x branch.
>> >
>> > regards,
>> >
>> > Leonardo Uribe
>> >
>> > 2011/7/11 Jakob Korherr <jakob.korherr@gmail.com>:
>> >> Hi,
>> >>
>> >> No, sorry Leo, they are not enough. Frankly, I don't understand why
>> >> you are blocking this solution. It is easy, it does not break anything
>> >> (if we do not change the package names), it is a lot more clean and we
>> >> can get rid of the shaded-impl module. If we don't do this now, we
>> >> will maybe have to use this ugly module for a long time..
>> >>
>> >> And yes: in my opinion it is an epic fail. If you think otherwise,
>> >> that's ok, but just because you say so and I don't does not make your
>> >> statement true.
>> >>
>> >> I think we have to start a vote for this one just like we did with the
>> >> resource-handler stuff.
>> >>
>> >> Regards,
>> >> Jakob
>> >>
>> >> 2011/7/11 Leonardo Uribe <lu4242@gmail.com>:
>> >>> Hi
>> >>>
>> >>> 2011/7/11 Jakob Korherr <jakob.korherr@gmail.com>:
>> >>>> Hi,
>> >>>>
>> >>>>> 1. All classes in org.apache.myfaces.spi.
>> >>>>
>> >>>> I did not change anything here, just internal imports (from
>> >>>> *.spi.impl
>> >>>> to *.spi.util)!
>> >>>
>> >>> It is questionable to create .spi.util. After all, it is not supposed
>> >>> that package be consumed by container integration code, so it should
>> >>> be on spi.impl.
>> >>>
>> >>>>
>> >>>>> 2. All classes that has to be with LifecycleProvider (@PostConstruct
>> >>>>> annotation). Such classes should be on spi package, but note
spi
>> >>>>> work
>> >>>>> started after 2.0 release, so this should wait to a major version.
>> >>>>
>> >>>> Geronimo (for which we did the SPI stuff) includes MyFaces 2.0.x.
>> >>>> Here
>> >>>> we are talking about 2.1.x. Furthermore, one call to organize imports
>> >>>> does the trick, so I do not see a problem here.
>> >>>
>> >>> Look the page of JSR-314 spec http://www.jcp.org/en/jsr/summary?id=314
>> >>>
>> >>> You will notice 2.1.x is not a new JSR like the future JSR-344 (JSF
>> >>> 2.2), and instead is tagged as "Maintenance Release 2". So, to be
>> >>> consistent, it should be possible to change between 2.0 and 2.1 on the
>> >>> same server. That's the most important reason to be so conservative
or
>> >>> strict with 2.1.
>> >>>
>> >>>>
>> >>>>> 3. All classes on org.apache.myfaces.config.element. These classes
>> >>>>> are
>> >>>>> an interface to manipulate faces-config.xml files with java
code,
>> >>>>> and
>> >>>>> spi interfaces provide the hooks to get them, so in theory we
can
>> >>>>> add
>> >>>>> methods there, but relocate this package to
>> >>>>> org.apache.myfaces.spi.data is not necessary. I think the package
>> >>>>> name
>> >>>>> is correct.
>> >>>>
>> >>>> OK, fine. I thought the relocation to org.apache.myfaces.spi.* would
>> >>>> fit better, since it is the myfaces-spi module and, again, since
>> >>>> we're
>> >>>> talking about 2.1.x, not 2.0.x here. But if you want to keep the
>> >>>> package-name, I have no problem with that.
>> >>>> org.apache.myfaces.config.element is fine too, however, you may
not
>> >>>> expect it to be in the myfaces-spi module.
>> >>>>
>> >>>>> [...] Considering this, I think the costs
>> >>>>> involved on the changes proposed are too big compared with the
>> >>>>> benefits, which are very limited and almost inexistent from
user
>> >>>>> point
>> >>>>> of view.
>> >>>>
>> >>>> From a user point of view: maybe yes. But from a developer point
of
>> >>>> view myfaces-shaded-impl is an epic fail. I know it was an easy
an
>> >>>> fast solution at the time we got 2.1.0 out, but for the long term
>> >>>> this
>> >>>> has to be changed. Please think about it, you use 2.0.5 (or any
other
>> >>>> _previous_ version) in myfaces-impl-ee6 as if it was 2.1.x-SNAPSHOT.
>> >>>> Thus you use internals of previous versions which might not even
be
>> >>>> there anymore in the current 2.1.x-SNAPSHOT. And the worst part
of
>> >>>> it:
>> >>>> you don't even see it at build time, b/c it's a separate module
and
>> >>>> myfaces-impl-ee6 is shaded into myfaces-impl (and shade does not
warn
>> >>>> about this kind of stuff, of course).
>> >>>>
>> >>>
>> >>> I know the hack done to compile 2.1 is not very clean, but it works.
>> >>> The idea is replace 2.0.5 ref with 2.1.0 in future versions. Note
>> >>> myfaces-shaded-impl is a module that is just for allow compile
>> >>> myfaces-impl-ee6, and nobody else. It is not "an epic fail", because
>> >>> it does not cause any changes on the code that cause problems.
>> >>>
>> >>>> Considering this, it was ok to create shaded-impl in order to get
the
>> >>>> 2.1.0 release out, but for future releases (maybe also towards
>> >>>> 2.2.0),
>> >>>> this must be done in another way.
>> >>>
>> >>> In fact, the idea is do something in 2.2.x, but that will take some
>> >>> time, so maybe we can keep in mind those changes until get there.
>> >>>
>> >>>> I have to say that I won't support a
>> >>>> 2.1.2 release including the shaded-impl module.
>> >>>
>> >>> I hope my arguments could be enough to convince you about the
>> >>> opposite.
>> >>>
>> >>> regards ,
>> >>>
>> >>> Leonardo Uribe
>> >>>
>> >>>>
>> >>>> Regards,
>> >>>> Jakob
>> >>>>
>> >>>> 2011/7/10 Leonardo Uribe <lu4242@gmail.com>:
>> >>>>> Hi Gerhard
>> >>>>>
>> >>>>> Ok, in theory the parts that we should not change, or otherwise
that
>> >>>>> will trigger a change on JEE containers are:
>> >>>>>
>> >>>>> 1. All classes in org.apache.myfaces.spi.
>> >>>>> 2. All classes that has to be with LifecycleProvider (@PostConstruct
>> >>>>> annotation). Such classes should be on spi package, but note
spi
>> >>>>> work
>> >>>>> started after 2.0 release, so this should wait to a major version.
>> >>>>> 3. All classes on org.apache.myfaces.config.element. These classes
>> >>>>> are
>> >>>>> an interface to manipulate faces-config.xml files with java
code,
>> >>>>> and
>> >>>>> spi interfaces provide the hooks to get them, so in theory we
can
>> >>>>> add
>> >>>>> methods there, but relocate this package to
>> >>>>> org.apache.myfaces.spi.data is not necessary. I think the package
>> >>>>> name
>> >>>>> is correct.
>> >>>>>
>> >>>>> regards,
>> >>>>>
>> >>>>> Leonardo Uribe
>> >>>>>
>> >>>>> 2011/7/9 Gerhard Petracek <gerhard.petracek@gmail.com>:
>> >>>>>> hi leo,
>> >>>>>> thx for checking it.
>> >>>>>> it would be great, if you can post a list of parts (not
classes)
>> >>>>>> which would
>> >>>>>> break. so we can discuss this topic in a more concrete manner.
>> >>>>>> regards,
>> >>>>>> gerhard
>> >>>>>> http://www.irian.at
>> >>>>>>
>> >>>>>> Your JSF powerhouse -
>> >>>>>> JSF Consulting, Development and
>> >>>>>> Courses in English and German
>> >>>>>>
>> >>>>>> Professional Support for Apache MyFaces
>> >>>>>>
>> >>>>>>
>> >>>>>> 2011/7/10 Leonardo Uribe <lu4242@gmail.com>
>> >>>>>>>
>> >>>>>>> Hi
>> >>>>>>>
>> >>>>>>> Good to know that. Unfortunately, do a change on the
spi related
>> >>>>>>> classes will break existing integration code with containers
like
>> >>>>>>> Geronimo, JBoss and others too. Considering this, I
think the
>> >>>>>>> costs
>> >>>>>>> involved on the changes proposed are too big compared
with the
>> >>>>>>> benefits, which are very limited and almost inexistent
from user
>> >>>>>>> point
>> >>>>>>> of view.
>> >>>>>>>
>> >>>>>>> regards,
>> >>>>>>>
>> >>>>>>> Leonardo Uribe
>> >>>>>>>
>> >>>>>>> 2011/7/8 Werner Punz <werner.punz@gmail.com>:
>> >>>>>>> > I have no problem if you break Ext-Script, after
all  I program
>> >>>>>>> > against
>> >>>>>>> > the
>> >>>>>>> > impl, so whatever change you do I have to patch
it in my
>> >>>>>>> > codebase
>> >>>>>>> > anyway.
>> >>>>>>> > Don´t feel stopped by it.
>> >>>>>>> >
>> >>>>>>> > Werner
>> >>>>>>> >
>> >>>>>>> >
>> >>>>>>> > Am 08.07.11 18:27, schrieb Gerhard Petracek:
>> >>>>>>> >>
>> >>>>>>> >> hi,
>> >>>>>>> >>
>> >>>>>>> >> thx for reviewing the changes.
>> >>>>>>> >> for sure we can discuss when we are going to
do such changes.
>> >>>>>>> >> however,
>> >>>>>>> >> if ext-script is the only reason against it,
we can do it right
>> >>>>>>> >> now
>> >>>>>>> >> imo.
>> >>>>>>> >> there is no problem with shipping a small update
of ext-script
>> >>>>>>> >> and the
>> >>>>>>> >> user base is currently small enough to communicate
this change
>> >>>>>>> >> easily.
>> >>>>>>> >>
>> >>>>>>> >> regards,
>> >>>>>>> >> gerhard
>> >>>>>>> >>
>> >>>>>>> >> http://www.irian.at
>> >>>>>>> >>
>> >>>>>>> >> Your JSF powerhouse -
>> >>>>>>> >> JSF Consulting, Development and
>> >>>>>>> >> Courses in English and German
>> >>>>>>> >>
>> >>>>>>> >> Professional Support for Apache MyFaces
>> >>>>>>> >>
>> >>>>>>> >>
>> >>>>>>> >>
>> >>>>>>> >> 2011/7/8 Leonardo Uribe <lu4242@gmail.com
>> >>>>>>> >> <mailto:lu4242@gmail.com>>
>> >>>>>>> >>
>> >>>>>>> >>    Hi
>> >>>>>>> >>
>> >>>>>>> >>    In principle I agree with this change,
but after a quick
>> >>>>>>> >> patch
>> >>>>>>> >> review
>> >>>>>>> >>    I saw some package relocation for some
classes (from
>> >>>>>>> >>    org.apache.myfaces.config.element to
>> >>>>>>> >> org.apache.myfaces.spi.data).
>> >>>>>>> >>    Those changes will break extensions scripting
code. Such
>> >>>>>>> >> changes
>> >>>>>>> >>    should be done between major versions
(2.2.0). Please do not
>> >>>>>>> >> change
>> >>>>>>> >>    the package names.
>> >>>>>>> >>
>> >>>>>>> >>    regards,
>> >>>>>>> >>
>> >>>>>>> >>    Leonardo Uribe
>> >>>>>>> >>
>> >>>>>>> >>    2011/7/8 Gerhard Petracek <gerhard.petracek@gmail.com
>> >>>>>>> >>    <mailto:gerhard.petracek@gmail.com>>:
>> >>>>>>> >>     > +1
>> >>>>>>> >>     > regards,
>> >>>>>>> >>     > gerhard
>> >>>>>>> >>     >
>> >>>>>>> >>     > http://www.irian.at
>> >>>>>>> >>     >
>> >>>>>>> >>     > Your JSF powerhouse -
>> >>>>>>> >>     > JSF Consulting, Development and
>> >>>>>>> >>     > Courses in English and German
>> >>>>>>> >>     >
>> >>>>>>> >>     > Professional Support for Apache
MyFaces
>> >>>>>>> >>     >
>> >>>>>>> >>     >
>> >>>>>>> >>     >
>> >>>>>>> >>     > 2011/7/8 Jakob Korherr <jakob.korherr@gmail.com
>> >>>>>>> >>    <mailto:jakob.korherr@gmail.com>>
>> >>>>>>> >>     >>
>> >>>>>>> >>     >> Hi guys,
>> >>>>>>> >>     >>
>> >>>>>>> >>     >> We currently use the shaded-impl
module in core 2.1.x to
>> >>>>>>> >> solve a
>> >>>>>>> >>     >> cyclic dependency problem. However,
this introduces
>> >>>>>>> >> redundancy
>> >>>>>>> >> and
>> >>>>>>> >>     >> complexity and may lead to some
strange issues. This can
>> >>>>>>> >> be
>> >>>>>>> >>    solved by
>> >>>>>>> >>     >> the introduction of a myfaces-spi
module (which is then
>> >>>>>>> >> packed
>> >>>>>>> >>     >> together with myfaces-impl,
just like myfaces-impl-ee6
>> >>>>>>> >> is).
>> >>>>>>> >>     >>
>> >>>>>>> >>     >> Please see MYFACES-3211 [1]
for a detailed description
>> >>>>>>> >> of the
>> >>>>>>> >>    solution
>> >>>>>>> >>     >> and a patch for it.
>> >>>>>>> >>     >>
>> >>>>>>> >>     >> If there are no objections,
I will commit this patch
>> >>>>>>> >> soon!
>> >>>>>>> >>     >>
>> >>>>>>> >>     >> Regards,
>> >>>>>>> >>     >> Jakob
>> >>>>>>> >>     >>
>> >>>>>>> >>     >> [1] https://issues.apache.org/jira/browse/MYFACES-3211
>> >>>>>>> >>     >>
>> >>>>>>> >>     >> --
>> >>>>>>> >>     >> Jakob Korherr
>> >>>>>>> >>     >>
>> >>>>>>> >>     >> blog: http://www.jakobk.com
>> >>>>>>> >>     >> twitter: http://twitter.com/jakobkorherr
>> >>>>>>> >>     >> work: http://www.irian.at
>> >>>>>>> >>     >
>> >>>>>>> >>     >
>> >>>>>>> >>
>> >>>>>>> >>
>> >>>>>>> >
>> >>>>>>> >
>> >>>>>>> >
>> >>>>>>
>> >>>>>>
>> >>>>>
>> >>>>
>> >>>>
>> >>>>
>> >>>> --
>> >>>> Jakob Korherr
>> >>>>
>> >>>> blog: http://www.jakobk.com
>> >>>> twitter: http://twitter.com/jakobkorherr
>> >>>> work: http://www.irian.at
>> >>>>
>> >>>
>> >>
>> >>
>> >>
>> >> --
>> >> Jakob Korherr
>> >>
>> >> blog: http://www.jakobk.com
>> >> twitter: http://twitter.com/jakobkorherr
>> >> work: http://www.irian.at
>> >>
>> >
>>
>>
>>
>> --
>> Jakob Korherr
>>
>> blog: http://www.jakobk.com
>> twitter: http://twitter.com/jakobkorherr
>> work: http://www.irian.at
>
>

Mime
View raw message