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 16:36:00 GMT
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