cordova-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jesse <purplecabb...@gmail.com>
Subject Re: [JS] merging to master for cordova-js and cordova-plugman
Date Mon, 28 Apr 2014 17:27:03 GMT
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