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 <[email protected]> wrote: > On Fri, Apr 25, 2014 at 7:35 PM, Michal Mocny <[email protected]> wrote: > > > On Fri, Apr 25, 2014 at 10:20 PM, purplecabbage <[email protected] > > >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 <[email protected]> > wrote: > > > > > > > > On Fri, Apr 25, 2014 at 11:05 AM, Andrew Grieve < > [email protected] > > > >wrote: > > > > > > > >> On Fri, Apr 25, 2014 at 1:35 PM, Michal Mocny <[email protected]> > > > wrote: > > > >>>> On Fri, Apr 25, 2014 at 1:27 PM, Anis KADRI <[email protected] > > > > > >>> 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 < > > [email protected] > > > >>>> wrote: > > > >>>> > > > >>>>> reverted the merge commits to plugman & js. > > > >>>>> > > > >>>>> On Fri, Apr 25, 2014 at 9:43 AM, Michal Mocny < > [email protected] > > > > > > >>>>> 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 < > > [email protected] > > > > > > > >>>>>> wrote: > > > >>>>>> > > > >>>>>>> > > > >>>>>>> > > > >>>>>>> > > > >>>>>>>> On Fri, Apr 25, 2014 at 12:53 AM, Michal Mocny < > > > [email protected] > > > >>>>>>> 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 < > > > >> [email protected] > > > >>>>>> 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 <[email protected]> > > > wrote: > > > >>>>>>>>>> \o/ > > > >>>>>>>>>> > > > >>>>>>>>>> > > > >>>>>>>>>> On Thu, Apr 24, 2014 at 2:30 PM, Anis KADRI < > [email protected]> > > > >>>>> 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 > > > >> > > > > > >
