myfaces-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gerhard Petracek <gerhard.petra...@gmail.com>
Subject Re: Proposal: Get rid of shaded-impl module in core 2.1.x
Date Mon, 11 Jul 2011 22:09:03 GMT
hi leo,

if both of you agree on the rest, we won't need a vote - that's right.

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/12 Leonardo Uribe <lu4242@gmail.com>

> 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