cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Grieve <agri...@chromium.org>
Subject Re: [JS] merging to master for cordova-js and cordova-plugman
Date Mon, 28 Apr 2014 18:59:35 GMT
I like that style is enforced with a tool, but I wouldn't be bothered if
was removed.


On Mon, Apr 28, 2014 at 1:27 PM, Jesse <purplecabbage@gmail.com> wrote:

> The fact that indentation can break the build is troubling to me. Am I
> alone?
>
> @purplecabbage
> risingj.com
>
>
> On Mon, Apr 28, 2014 at 9:36 AM, Andrew Grieve <agrieve@chromium.org>
> wrote:
>
> > I didn't revert it right away, I sent an email saying that I'd do it in
> the
> > morning if it wasn't fixed, and then did so. It's much worse to have a
> > broken build than it is to just revert. It's *very* easy to revert a
> revert
> > commit, and it takes the pressure off the person who broke it to come up
> > with a fix quickly.
> >
> > Agree I could have just fixed the indents. I didn't realize that was the
> > only thing that was wrong with it.
> >
> >
> >
> >
> > On Sat, Apr 26, 2014 at 1:25 AM, Anis KADRI <anis.kadri@gmail.com>
> wrote:
> >
> > > On Fri, Apr 25, 2014 at 7:35 PM, Michal Mocny <mmocny@chromium.org>
> > wrote:
> > >
> > > > On Fri, Apr 25, 2014 at 10:20 PM, purplecabbage <
> > purplecabbage@gmail.com
> > > > >wrote:
> > > >
> > > > > I believe the plan was, merge now to save headaches later *but*
> also
> > > hide
> > > > > the browserify functionality behind a flag, so it could co-exist.
> > > > >
> > > > > Sent from my iPhone
> > > > >
> > > > > > On Apr 25, 2014, at 5:40 PM, Anis KADRI <anis.kadri@gmail.com>
> > > wrote:
> > > > > >
> > > > > > On Fri, Apr 25, 2014 at 11:05 AM, Andrew Grieve <
> > > agrieve@chromium.org
> > > > > >wrote:
> > > > > >
> > > > > >> On Fri, Apr 25, 2014 at 1:35 PM, Michal Mocny <
> > mmocny@chromium.org>
> > > > > wrote:
> > > > > >>>> On Fri, Apr 25, 2014 at 1:27 PM, Anis KADRI <
> > anis.kadri@gmail.com
> > > >
> > > > > >>> wrote:
> > > > > >>>
> > > > > >>>> Contacts is known to not work because some modules
> > > > > >>>> require('./ContactError') instead of
> > > > > >>>> ('org.apache.cordova.contacts.ContactError') like
most other
> > > modules
> > > > > >> do. I
> > > > > >>>> didn't bother to fix yet it because it was an exception
not a
> > > rule.
> > > > > >>>> I didn't try any of the chrome plugins. They might
be doing
> the
> > > same
> > > > > >> sort
> > > > > >>>> of thing.
> > > > > >>
> > > > > >> Just wanted to be clear here that "fix" means allow the
> > "./syntax",
> > > > > >> not convert to abs syntax. File plugin uses this extensively,
> and
> > > > > >> we've supported it for quite a while.
> > > > > >
> > > > > > Thanks for the heads up but require('path') works. The only
> > > difference
> > > > is
> > > > > > that it actually checks if the path exists and if it doesn't
then
> > it
> > > > > fails.
> > > > > > Which makes this file [1] fail because it's requiring a module
> that
> > > is
> > > > > > present in the parent directory (../ContactError) while
> referencing
> > > it
> > > > as
> > > > > > if it was in the same directory (./ContactError). Are chrome
> > plugins
> > > > > doing
> > > > > > a similar thing ? File/FileTransfer are not concerned because
the
> > > paths
> > > > > > they reference do exist.
> > > > > >
> > > > > >
> > > > > >>
> > > > > >>
> > > > > >>>> I didn't noticed any major performance issues when
comparing
> old
> > > > > plugman
> > > > > >>>> and new plugman with browserify. I am not using
CLI though.
> > > > > >>
> > > > > >> As a litmus test, I'd suggest running the createmobilespec.js
> > script
> > > > > >> within mobile-spec.
> > > > > >
> > > > > > I will try this out. The reason it takes longer is partly because
> > > it's
> > > > > > generating 2 bundles. I am sure there can be ways to improve
> > > > performance
> > > > > > though. I will keep looking into it.
> > > > > >
> > > > > >
> > > > > >>
> > > > > >>>> Thanks for giving me the time to look into this
before
> reverting
> > > my
> > > > > >>>> changes. Much appreciated. Besides reverting cordova-js
was
> > > > completely
> > > > > >>>> unnecessary because I only added a task to compile
a
> browserify
> > > > bundle
> > > > > >>>> (everything else was not touched).
> > > > > >>
> > > > > >> I think it's important to keep the continuous build green,
and
> > > > > >> cordova-js was broken:
> > > > > >> https://issues.apache.org/jira/browse/CB-6515
> > > > > >>
> > > > > >> To re-do your changes, just git revert the revert commit
and
> carry
> > > on.
> > > > > >> It's important that everyone else isn't blocked by master
being
> > > > > >> broken.
> > > > > >
> > > > > > jshint was failling because I used 2 indentations instead of
4.
> > > Indeed,
> > > > > > that is absolutely unacceptable.
> > > > >
> > > >  > I suggest that we make indentation our top priority going forward.
> > > Those
> > > > > > types of bug that users report that make them unable to compile
> > their
> > > > > > projects can just sit in there and wait after all
> > > > > > (CB-6441<https://issues.apache.org/jira/browse/CB-6441>).
> > > > > > Who cares about real users as long as the continuous build screen
> > is
> > > > > green,
> > > > > > right?
> > > > >
> > > >
> > > >
> > > > Why did you include that fix as part of this unrelated work?  If you
> > > > squashed the browserify work it would have been easier to pick out
> more
> > > > specific reverts.  (additionally there is duplication of all the
> > > browserify
> > > > commits if you look at the git log, not sure how that happened, so
> > revert
> > > > of anything but the whole ).
> > >
> > >
> > > Could you please clarify your thoughts? I don't get this part.
> > >
> > >
> > > >
> > > >
> > > > > >
> > > > > > I fixed the indentation which makes jshint happy. I merged the
> > > > browserify
> > > > > > branch back to master (with the critical indentation fix). You
do
> > let
> > > > me
> > > > > > know if your continuous build is bleeding again.
> > > > >
> > > >
> > > > Dude its not "your" CI, its our CI.  It catches real issues weekly,
> and
> > > > cannot do that if its red.
> > > >
> > >
> > > Ownership doesn't matter. I never criticized the CI and same as you, I
> > > think it can be really useful. I just said that what it caught this
> time
> > > was absolute BS and it would have been easier to fix it than to revert
> > > everything. So the first reflex when it's red should always be revert ?
> > No
> > > chance/time to fix ? It took me less 20 seconds (ok maybe a minute).
> > >
> > >
> > > >
> > > >
> > > > > >
> > > > > > Finally, you asked me at the last hangout to merge everything
to
> > > master
> > > > > in
> > > > > > order to survive the breakout of cordova-cli and cordova-plugman
> > and
> > > > that
> > > > > > is why I took the liberty to do it sooner rather than later.
Also
> > we
> > > > need
> > > > > > to start testing this stuff at some point and it's not like
> nothing
> > > has
> > > > > > ever been broken with the project.
> > > > >
> > > >
> > > > ^ What Jesse said.
> > > >
> > >
> > > What Jesse said is already there for cordova-js (there is a compile and
> > > compile-browserify task that don't overlap in any way). There is no
> flag
> > > for plugman and perhaps there should be. That is a valid point. So I
> will
> > > make it available behind a flag and we'll go from there. But you see
> > these
> > > are the discussions that I was hoping to trigger when I sent this
> > message.
> > > I was not expecting a: "!@#% it, it's broken, let's revert cuz I don't
> > want
> > > to see my CI bleed more than 5 minutes". What was the urgency anyway ?
> > Were
> > > we releasing today?
> > >
> > >
> > > >
> > > >
> > > > >  >
> > > > > > [1]
> > > > > >
> > > > >
> > > >
> > >
> >
> https://git-wip-us.apache.org/repos/asf?p=cordova-plugin-contacts.git;a=blob;f=www/ios/Contact.js;h=b40c41acc5c78e2233ca434388ab012cd635fba9;hb=HEAD
> > > > > >
> > > > > >
> > > > > >>
> > > > > >>>
> > > > > >>> All the fixes will probably take more than an hour,
and
> shouldn't
> > > > block
> > > > > >>> master.  Why the rush to merge in?
> > > > > >>>
> > > > > >>>
> > > > > >>>>
> > > > > >>>>
> > > > > >>>>
> > > > > >>>>> On Fri, Apr 25, 2014 at 8:13 AM, Andrew Grieve
<
> > > > agrieve@chromium.org
> > > > > >>>> wrote:
> > > > > >>>>
> > > > > >>>>> reverted the merge commits to plugman &
js.
> > > > > >>>>>
> > > > > >>>>> On Fri, Apr 25, 2014 at 9:43 AM, Michal Mocny
<
> > > mmocny@chromium.org
> > > > >
> > > > > >>>>> wrote:
> > > > > >>>>>> Also Anis, would it please be possible to
squash most of
> these
> > > > > >> commits
> > > > > >>>>> into
> > > > > >>>>>> a few units of standalone work?  Probably
best way to do
> that
> > is
> > > > > >> create
> > > > > >>>>> a
> > > > > >>>>>> new local branch from your old branch point,
apply the diffs
> > > from
> > > > > >>>>>> browserify branch, rebase -i them, then
push that branch
> (this
> > > way
> > > > > >> you
> > > > > >>>>>> don't need to push with force).
> > > > > >>>>>>
> > > > > >>>>>> -Michal
> > > > > >>>>>>
> > > > > >>>>>>
> > > > > >>>>>>> On Fri, Apr 25, 2014 at 9:30 AM, Michal
Mocny <
> > > > mmocny@chromium.org
> > > > > >
> > > > > >>>>>> wrote:
> > > > > >>>>>>
> > > > > >>>>>>>
> > > > > >>>>>>>
> > > > > >>>>>>>
> > > > > >>>>>>>> On Fri, Apr 25, 2014 at 12:53 AM,
Michal Mocny <
> > > > > mmocny@chromium.org
> > > > > >>>>>>> wrote:
> > > > > >>>>>>>
> > > > > >>>>>>>> TLDR; Ran a bunch of experiments
tonight.  I think this is
> > too
> > > > > >> early
> > > > > >>>>> to
> > > > > >>>>>>>> merge into master.  We pointed out
several issues in
> > previous
> > > > > >> threads
> > > > > >>>>> and
> > > > > >>>>>>>> seems they were ignored.
> > > > > >>>>>>>>
> > > > > >>>>>>>> Few quick comments from trying this:
> > > > > >>>>>>>> - cordova-js is a new dependency
of plugman, and needs to
> be
> > > npm
> > > > > >>>>> linked
> > > > > >>>>>>>> to local dev version
> > > > > >>>>>>>> - Seems we run browserify after
each plugin add (possibly
> > due
> > > to
> > > > > an
> > > > > >>>>>>>> auto-prepare?) so creating projects
like mobilespec or any
> > > > mobile
> > > > > >>>>> chrome
> > > > > >>>>>>>> app is now *much* slower (measured
in minutes)
> > > > > >>>>>>>>  - Each cordova prepare now takes
6.5s on a very small
> > project
> > > > :'(
> > > > > >>>>>>> (reverted changes locally, old prepare
takes <0.5s on same
> > > > project,
> > > > > >>>>> which
> > > > > >>>>>>> is still slow!)
> > > > > >>>>>>>
> > > > > >>>>>>>
> > > > > >>>>>>>> Things that are currently found
broken:
> > > > > >>>>>>>> - prepare step fails for my cordova
testing application
> > after
> > > > > >>>>>>>> installing org.apache.cordova.contacts,
and
> > > > > >>>>>>>> - prepare step fails for *all* cca
apps because of same
> > error
> > > as
> > > > > >>>>> above,
> > > > > >>>>>>>> but for chrome.runtime plugin :(
> > > > > >>>>>>>>  - These issues seem due to js-modules
not being
> > browserify-ed
> > > > > >>>>> properly.
> > > > > >>>>>>>> It may be that both are bad modules
(?), but it used to
> work
> > > > fine!
> > > > > >>>>>>>>
> > > > > >>>>>>>> I did get a few apps running fine,
so at least we got that
> > > going
> > > > > >> for
> > > > > >>>>> us ;)
> > > > > >>>>>>>>
> > > > > >>>>>>>> Still to do:
> > > > > >>>>>>>> - track impact tp startup time
> > > > > >>>>>>>> - see if there aren't any plugins
with subtle bugs due to
> > > > > auto-runs
> > > > > >>>>>>>> behaviour
> > > > > >>>>>>>>
> > > > > >>>>>>>> -Michal
> > > > > >>>>>>>>
> > > > > >>>>>>>>
> > > > > >>>>>>>> On Thu, Apr 24, 2014 at 9:34 PM,
Andrew Grieve <
> > > > > >> agrieve@chromium.org
> > > > > >>>>>> wrote:
> > > > > >>>>>>>>
> > > > > >>>>>>>>> Cool! Does no impact mean that
browserify is still not
> used
> > > by
> > > > > >>>>>>>>> default, or does it mean that
it's backward compatible?
> > > > > >>>>>>>>>
> > > > > >>>>>>>>> Failing specs sounds like impact...
> > > > > >>>>>>>>>
> > > > > >>>>>>>>> And it does look like medic
is failing due to
> > browserify-type
> > > > > >> things:
> > > > > >>>>>>>>> http://108.170.217.131:8010/waterfall
> > > > > >>>>>>>>>
> > > > > >>>>>>>>> Unless you feel like powering
through this tonight, I'll
> > > > probably
> > > > > >>>>>>>>> revert in the morning so that
our continuous build can
> stay
> > > > > green.
> > > > > >>>>>>>>>
> > > > > >>>>>>>>>> On Thu, Apr 24, 2014 at
6:06 PM, Brian LeRoux <
> b@brian.io
> > >
> > > > > wrote:
> > > > > >>>>>>>>>> \o/
> > > > > >>>>>>>>>>
> > > > > >>>>>>>>>>
> > > > > >>>>>>>>>> On Thu, Apr 24, 2014 at
2:30 PM, Anis KADRI <
> > > anis@apache.org>
> > > > > >>>>> wrote:
> > > > > >>>>>>>>>>
> > > > > >>>>>>>>>>> I just merged both browserify
branches into master.
> There
> > > > > >> should
> > > > > >>>>> be no
> > > > > >>>>>>>>>>> impact.
> > > > > >>>>>>>>>>> Right now most specs
pass expect for File,
> FileTransfer,
> > > > Media
> > > > > >> and
> > > > > >>>>>>>>> Contacts
> > > > > >>>>>>>>>>> due to some issues with
merges/clobbers and I am
> looking
> > > into
> > > > > >>>>> those.
> > > > > >>>>>>>>>>>
> > > > > >>>>>>>>>>> Also, I got rid of the
project cache condition in
> plugman
> > > > that
> > > > > >> was
> > > > > >>>>>>>>>>> preventing iOS frameworks
from being added (CB-6441)
> > > > > >>>>>>>>>>>
> > > > > >>>>>>>>>>> Anis
> > > > > >>
> > > > >
> > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message