Bug 50299 - webkit-patch needs land-no-bug
Summary: webkit-patch needs land-no-bug
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: ToolsHitList
Depends on:
Blocks:
 
Reported: 2010-11-30 17:03 PST by Ojan Vafai
Modified: 2011-06-24 10:57 PDT (History)
6 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2010-11-30 17:03:14 PST
Is there any reason not to? It would have caught a mistake I made yesterday modifying chromium's test_expectations.txt file. I'm happy to make the change if there's no opposition to it.
Comment 1 Eric Seidel (no email) 2010-11-30 17:06:08 PST
I still can't believe people actually use that command.

Adam is the cowboy.  Talk to him.  That is, assuming cowboys talk?
Comment 2 Adam Barth 2010-11-30 17:14:26 PST
Cowboys don't care about style.  :)

Feel free to make a land-ojanstyle that does what you want.
Comment 3 Eric Seidel (no email) 2010-11-30 17:16:06 PST
Why do you want land-cowboy at all Ojan?  What does it offer you over land?
Comment 4 Adam Barth 2010-11-30 17:41:23 PST
I think land-cowboy generates ChangeLogs.  It's like prepare+land, just like upload is prepare+post.
Comment 5 Ojan Vafai 2010-12-01 15:48:57 PST
I use webkit-patch land-cowboy as more of a "land-without-a-corresponding-bug" or "land-without-review". This comes up frequently for chromium webkit gardeners as they need to do a lot of commits without review (e.g. to test_expectations.txt). In those cases, running check-style is useful.

Is there a benefit of land-cowboy not running check-style? Is there another command I should be using? Should I add "land-without-review" or something?

So, I want something that:
generates changelog
runs check-style
commits
Comment 6 Adam Barth 2010-12-01 15:51:53 PST
> Is there a benefit of land-cowboy not running check-style?

It's faster and it doesn't complain when you make style errors.  Cowboys don't care about style errors.

> Is there another command I should be using? Should I add "land-without-review" or something?

Yeah, I think you should add a new command that does exactly what you want.  Commands (especially hidden commands) are cheap.
Comment 7 Dirk Pranke 2010-12-01 15:53:18 PST
(In reply to comment #5)
> So, I want something that:
> generates changelog
> runs check-style
> commits

Isn't that just 'webkit-patch land' ?
Comment 8 Adam Barth 2010-12-01 15:56:12 PST
(In reply to comment #7)
> (In reply to comment #5)
> > So, I want something that:
> > generates changelog
> > runs check-style
> > commits
> 
> Isn't that just 'webkit-patch land' ?

land doesn't create a ChangeLog or run check-style.
Comment 9 Ojan Vafai 2010-12-01 15:59:34 PST
(In reply to comment #6)
> > Is there a benefit of land-cowboy not running check-style?
> 
> It's faster and it doesn't complain when you make style errors.  Cowboys don't care about style errors.

Why do we want the tool to make committing style errors easy? I don't support cowboys apparently.


(In reply to comment #7)
> Isn't that just 'webkit-patch land' ?

webkit-patch land:
        steps.EnsureBuildersAreGreen,
        steps.UpdateChangeLogsWithReviewer,
        steps.ValidateReviewer,
        steps.Build,
        steps.RunTests,
        steps.Commit,
        steps.CloseBugForLandDiff,

webkit-patch land-cowboy:
        steps.PrepareChangeLog,
        steps.EditChangeLog,
        steps.ConfirmDiff,
        steps.Build,
        steps.RunTests,
        steps.Commit,

proposed webkit-patch land-no-bug:
        steps.PrepareChangeLog,
        steps.EditChangeLog,
        steps.CheckStyle,
        steps.ConfirmDiff,
        steps.Build,
        steps.RunTests,
        steps.Commit,
Comment 10 Dirk Pranke 2010-12-01 16:09:00 PST
(In reply to comment #9)
> (In reply to comment #6)
> > > Is there a benefit of land-cowboy not running check-style?
> > 
> > It's faster and it doesn't complain when you make style errors.  Cowboys don't care about style errors.
> 
> Why do we want the tool to make committing style errors easy? I don't support cowboys apparently.
> 

+1

> 
> (In reply to comment #7)
> > Isn't that just 'webkit-patch land' ?
> 
> webkit-patch land:
>         steps.EnsureBuildersAreGreen,
>         steps.UpdateChangeLogsWithReviewer,
>         steps.ValidateReviewer,
>         steps.Build,
>         steps.RunTests,
>         steps.Commit,
>         steps.CloseBugForLandDiff,
> 
> webkit-patch land-cowboy:
>         steps.PrepareChangeLog,
>         steps.EditChangeLog,
>         steps.ConfirmDiff,
>         steps.Build,
>         steps.RunTests,
>         steps.Commit,
> 
> proposed webkit-patch land-no-bug:
>         steps.PrepareChangeLog,
>         steps.EditChangeLog,
>         steps.CheckStyle,
>         steps.ConfirmDiff,
>         steps.Build,
>         steps.RunTests,
>         steps.Commit,

It's not clear to me why webkit-patch land doesn't do the missing steps, since these steps tend to be idempotent. It would seem to me like if you didn't want to step through the prompts, then that's what --non-interactive should be for.

Also, when I do webkit-patch land, it never does a build or runs tests. I don't know offhand what to make of that discrepancy.
Comment 11 Adam Barth 2010-12-01 16:31:23 PST
> Why do we want the tool to make committing style errors easy? I don't support cowboys apparently.

Yeah, I suspect you're just not cowboy enough for that command.  ;)

> It's not clear to me why webkit-patch land doesn't do the missing steps, since these steps tend to be idempotent. It would seem to me like if you didn't want to step through the prompts, then that's what --non-interactive should be for.

land is optimized for patches that have already been reviewed via the normal WebKit process.  Such patches already have ChangeLogs and have been checked for style by both humans and bots.

> Also, when I do webkit-patch land, it never does a build or runs tests. I don't know offhand what to make of that discrepancy.

You can have it build and test using the --build and --test flags, respectively.  They used to be on by default but were turned off by default by popular demand.
Comment 12 Dirk Pranke 2010-12-01 16:35:36 PST
(In reply to comment #11)
> > It's not clear to me why webkit-patch land doesn't do the missing steps, since these steps tend to be idempotent. It would seem to me like if you didn't want to step through the prompts, then that's what --non-interactive should be for.
> 
> land is optimized for patches that have already been reviewed via the normal WebKit process.  Such patches already have ChangeLogs and have been checked for style by both humans and bots.
>

It doesn't seem like doing the steps anyway makes things much slower for 'webkit-patch upload'. Would it significantly slow down land?
 
> > Also, when I do webkit-patch land, it never does a build or runs tests. I don't know offhand what to make of that discrepancy.
> 
> You can have it build and test using the --build and --test flags, respectively.  They used to be on by default but were turned off by default by popular demand.

I see.

We should add a "webkit-patch list-steps 'command'" :)
Comment 13 Eric Seidel (no email) 2010-12-02 15:10:50 PST
webkit-patch help 'command' does that, sorta.  I had thoughts for a while of making it list the steps, but never did.
Comment 14 Ojan Vafai 2010-12-06 12:28:50 PST
(In reply to comment #6)
> > Is there a benefit of land-cowboy not running check-style?
> 
> It's faster and it doesn't complain when you make style errors.  Cowboys don't care about style errors.

Coming back to this...I still don't see the point. By that argument, why do we even confirm the diff? Check-style is fast, no? Anyways, you can do:

webkit-patch land-cowboy --ignore-style

I could just add my own command of course, but I don't agree that the cost is low just because it's hidden by default. Chromium webkit gardeners need to use this command frequently and, for that case style checking by default is especially important so that test_expectations changes get checked to avoid breaking the bots.
Comment 15 Eric Seidel (no email) 2011-06-24 10:04:25 PDT
Did you decide to write your own?  Should we close this?  We can always add more hidden webkit-patch commands.  Feel free to add a land-ojan.
Comment 16 Ojan Vafai 2011-06-24 10:57:12 PDT
I won't be getting to this anytime soon. No need to keep the bug open.