cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anis KADRI <anis.ka...@gmail.com>
Subject Re: [JS] merging to master for cordova-js and cordova-plugman
Date Sat, 26 Apr 2014 05:25:07 GMT
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