Bug 107724 – check-webkit-style says "Have to enable auto props in the subversion config file"

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Christian Biesinger]

https://bugs.webkit.org/show_bug.cgi?id=9221#c5

The style review bot says I need to adjust the subversion config. Clearly, the bot should not have that message.

[Eric Seidel (no email)]

I don't think this check belongs in check-webkit-style.  I think it should be moved to some pre-commit check instead.

[Dominik Röttsches (drott)]

Similar here, https://bugs.webkit.org/show_bug.cgi?id=107548#c2

[Alexey Proskuryakov]

Can you please elaborate on the rationale? It's clear to you, but not to me.

Patches include MIME type information, so what's wrong with catching mistakes early?

[Eric Seidel (no email)]

(In reply to comment #3)
> Can you please elaborate on the rationale? It's clear to you, but not to me.
> 
> Patches include MIME type information, so what's wrong with catching mistakes early?

Oh, the problem is this is check-webkit-style yelling about a configuration issue, on the machine.  It's OK for us to do that on users's machines, but it doesn't make much sense on the bot.

I'm not certain that git patches do include mimetype info?  Or at least it's not clear to me why some people's patches go crazy and others don't.

The CQ should be configured to land these types of files with the right mime-types anyway, so only manual land users should run into this.

[Alexey Proskuryakov]

> Oh, the problem is this is check-webkit-style yelling about a configuration issue, on the machine.  It's OK for us to do that on users's machines, but it doesn't make much sense on the bot.

I see. So it's the phrasing of the complaint that is the issue - it should say that MIME type is wrong in the patch, and possibly suggest using auto-props. Obviously, the tool has no crystal ball to tell what's happening at user's machine - they may as well have auto-props, but manually remove MIME types from patches before submitting them :).

[Christian Biesinger]

That's not what the check is doing though. It literally checks the subversion config file, not the patch. See Tools/Scripts/webkitpy/common/checksvnconfigfile.py as used by Tools/Scripts/webkitpy/style/checkers/png.py

[Alexey Proskuryakov]

Oh. That makes no sense to me either.

[Alan Cutter]

I have updated the .subversion/config file on gce-feeder-01 (where the style queue lives currently).

I agree it does seem strange for the style checker to be checking its own machine instead of the patch directly but I have no qualms against simply making sure the bots are set up correctly.

[Alexey Proskuryakov]

*** Bug 107862 has been marked as a duplicate of this bug. ***

[Christian Biesinger]

*** Bug 110249 has been marked as a duplicate of this bug. ***

[Christian Biesinger]

Alan, looks like this is happening again: https://bugs.webkit.org/show_bug.cgi?id=109994#c6

[Alan Cutter]

Created attachment 189454 [details]
Patch

[Alan Cutter]

(In reply to comment #11)
> Alan, looks like this is happening again: https://bugs.webkit.org/show_bug.cgi?id=109994#c6

Thanks for notifying me. Fixed up the subversion config file on gce-feeder-01 again.


(In reply to comment #12)
> Created an attachment (id=189454) [details]
> Patch

Submitting patch to resolve issue.

[Eric Seidel (no email)]

Why all the boiler-plate?

Should we just have a standard svn-config which we recommend for the project?

[Alan Cutter]

(In reply to comment #14)
> Why all the boiler-plate?
> 
> Should we just have a standard svn-config which we recommend for the project?

A standard svn config file sound like a good idea, I'm not familiar with WebKit's directory structure enough to decide where to put it.

By boilerplate do you mean all the comments that come with the config file? I decided to leave them in as I can only see them being useful when doing EWS bot maintenance.

As an alternative to storing the svn config file in the repo for developers I think adding instructions for enabling the particular SVN flags on the WebKit wiki could be sufficient.
https://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree
Possibly adding a section under Pixel test tips.

[Eric Seidel (no email)]

Comment on attachment 189454 [details]
Patch

No, I think we definitely want a standard recommended config file in the repo.  I just think the boiler-plate is a bit silly. :)  This is just a two-line file, no?  Seems we also want .sln files, etc, no?

[Eric Seidel (no email)]

CCing folks who may have an opinion about a standard svnconfig file for WebKit.

[Tony Chang]

vcproj, sln and vsprops files should be svn:eol-style native.  See bug 96934 for a discussion of this.

Checking in a config file and having the style check suggest using it seems fine to me.  Not sure it's worthwhile to keep all the commented out lines in the same config file.

It seems like it was good that this check was run on the bots, otherwise we wouldn't have known to fix the bots.

[Alan Cutter]

If we include a svn config file in the repo should we also include a script to merge it with the user's existing svn repo? This might be a bit invasive. Perhaps documentation on the Wiki is suitable enough.

In terms of where the svn config file should go does Tools/config/svn-config sound appropriate?
As far as I can tell there's no other directory for user config files.

[Alan Cutter]

(In reply to comment #19)
> If we include a svn config file in the repo should we also include a script to merge it with the user's existing svn repo?

*user's existing svn config

[Eric Seidel (no email)]

I suspect most users don't have a config to start with.  They probably have one for WebKit, because it's needed for the silly png thing, but having a cannonical config to recommend when setting up new machines is the right long-term solution IMO.

[Tony Chang]

(In reply to comment #19)
> If we include a svn config file in the repo should we also include a script to merge it with the user's existing svn repo? This might be a bit invasive. Perhaps documentation on the Wiki is suitable enough.

Seems like overkill.  I think it's reasonable to do just check that the settings are correct and if they're incorrect, point the user to a wiki page or tell them how to fix.

> In terms of where the svn config file should go does Tools/config/svn-config sound appropriate?
> As far as I can tell there's no other directory for user config files.

I would probably put it at Tools/svn-config (or Tools/subversion-config) to avoid making a subdirectory with 1 file in it.

[Alan Cutter]

Created attachment 189639 [details]
Patch

[Alan Cutter]

(In reply to comment #23)
> Created an attachment (id=189639) [details]
> Patch

Removed comments in svn-config file.

Placed svn-config under Tools/

Added
*.sln = svn:eol-style=native
*.vcproj = svn:eol-style=native
*.vsprops = svn:eol-style=native
to svn-config.

Updated bot scripts to use new location.

[Tony Chang]

Comment on attachment 189639 [details]
Patch

LGTM.  I don't feel strongly about the location of the file, but someone else might.

[WebKit Review Bot]

Comment on attachment 189639 [details]
Patch

Clearing flags on attachment: 189639

Committed r144716: <http://trac.webkit.org/changeset/144716>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Viatcheslav Ostapenko]

(In reply to comment #8)
> I have updated the .subversion/config file on gce-feeder-01 (where the style queue lives currently).
> 
> I agree it does seem strange for the style checker to be checking its own machine instead of the patch directly but I have no qualms against simply making sure the bots are set up correctly.

Still style check fails for pngs on EWS bot.
Thought that git doesn't add mime type info and created patch in svn tree and checked that mime type is included, but it didn't help.
Here: https://bugs.webkit.org/show_bug.cgi?id=110620

[Alan Cutter]

(In reply to comment #28)
> (In reply to comment #8)
> > I have updated the .subversion/config file on gce-feeder-01 (where the style queue lives currently).
> > 
> > I agree it does seem strange for the style checker to be checking its own machine instead of the patch directly but I have no qualms against simply making sure the bots are set up correctly.
> 
> Still style check fails for pngs on EWS bot.
> Thought that git doesn't add mime type info and created patch in svn tree and checked that mime type is included, but it didn't help.
> Here: https://bugs.webkit.org/show_bug.cgi?id=110620

Thanks for reporting this. I'm positive the config was updated on the style bot but upon checking it was back to the default. I've updated it again to the required settings. Please try your patch again, apologies for the inconvenience!