Bug 98152 – Remove comments from the Apache config files to start avoiding useless duplication.

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

[Raphael Kubo da Costa (:rakuco)]

Remove comments from the Apache config files to start avoiding useless duplication.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 166678 [details]
Patch

[Laszlo Gombos]

I personally find the comments in the config file useful to keep, but I understand the intention here. Would you consider putting the comments (one version of the comments) back after the files are consolidated ?

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #2)
> I personally find the comments in the config file useful to keep, but I understand the intention here. Would you consider putting the comments (one version of the comments) back after the files are consolidated ?

That could be possible, but in the end I would like to trim the file down to the point that only a few directives are needed, preferably the ones everyone is most familiar with.

[Laszlo Gombos]

Comment on attachment 166678 [details]
Patch

Looks good to me, r+.

[Raphael Kubo da Costa (:rakuco)]

Thanks a lot. I will wait at least a few more hours before landing to give the guys in the West Coast some time to chime in as well.

[Eric Seidel (no email)]

We could also just generate these from python, or at least do our per-por modifications:
http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/servers/apache_http_server.py#L117

I'm glad to see someone cleaning this up a bit.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #6)
> We could also just generate these from python, or at least do our per-por modifications:
> http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/layout_tests/servers/apache_http_server.py#L117

Yes, after talking to Dirk yesterday my idea is to generate as much configuration from Python as possible and get rid of all those mostly duplicate files in LayouTests/http/conf.

[Eric Seidel (no email)]

SGTM.  I suspect each port requires very little custom config.

[Raphael Kubo da Costa (:rakuco)]

Err, webkit-patch land-safely seems to have done only half of its job. I will land this manually.

[Eric Seidel (no email)]

(In reply to comment #9)
> Err, webkit-patch land-safely seems to have done only half of its job. I will land this manually.

Half, how?  Coul dyou file a bug with the error?

[Raphael Kubo da Costa (:rakuco)]

Committed r130182: <http://trac.webkit.org/changeset/130182>

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #10)
> (In reply to comment #9)
> > Err, webkit-patch land-safely seems to have done only half of its job. I will land this manually.
> 
> Half, how?  Coul dyou file a bug with the error?

Actually, I think it is most likely my fault -- I assumed it was going to cq+ the reviewed patch, but it turns out it was going to upload my working diff instead (and I had none), so it only marked the current patch as obsolete and stopped.

[Eric Seidel (no email)]

Still sounds like a bug.  It should probably abort earlier if there is no diff. :(

[Eric Seidel (no email)]

(In reply to comment #13)
> Still sounds like a bug.  It should probably abort earlier if there is no diff. :(

Filed bug 98171.

[Ojan Vafai]

This caused inspector/debugger/source-url-comment.html to fail on the chromium bots. For some reason, these tests are loading tests from the http/tests directory. Not sure if that's related.

<script src="../../http/tests/inspector/inspector-test.js"></script>
<script src="../../http/tests/inspector/debugger-test.js"></script>

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=inspector%2Fdebugger%2Fsource-url-comment.html

[Ojan Vafai]

Nm...I think the bots were just confused about revision numbers.

[Alexey Proskuryakov]

I also used to find these comments helpful when playing with configuration files. Having lines even for default values is useful to know what you can change.

Why is trimming down the configuration files important?

[Eric Seidel (no email)]

These config files haven't changed much in years.  My understanding of what he's going for is to get us down to a single config file (which could have comments if we like) and then just some python to adjust the individual ports.

I suspect he'll find that we need very little configuration to support our needs.  I also suspect he'll find that the NRWT port architecture will make it very easy to do this all in python and either not have a config file at all (like Chromium currently does) or have a single one and a couple lines of per-port python tweaks.

[Eric Seidel (no email)]

I'm not really for or against the comments.  If folks feel strongly, I'm sure we can add them back, or better document our few needed config options.   (The comments have never really helped me.  Either a. I always need to google the Apache commands anyway, or b. I don't trust our files and look at the latest-and-greatest default config file from apache.org to make decisions on what to add/remove.)

[Alexey Proskuryakov]

> These config files haven't changed much in years.

It's not always landed changes. For example, I found myself changing these files locally to test a different authentication scheme.

I don't see the need to deviate from what's shipped with Apache by default. As you said, we don't hack on these files much.

[Raphael Kubo da Costa (:rakuco)]

I really don't have anything against putting comments back; the reason I committed this was just to make it easier to do the necessary plumbing, otherwise all changes to these files would result in big, cluttered diffs. I'm OK with adding comments back later, sure.