Bug 98152 - Remove comments from the Apache config files to start avoiding useless duplication.
Summary: Remove comments from the Apache config files to start avoiding useless duplic...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Raphael Kubo da Costa (:rakuco)
URL:
Keywords:
Depends on:
Blocks: 98151
  Show dependency treegraph
 
Reported: 2012-10-02 05:41 PDT by Raphael Kubo da Costa (:rakuco)
Modified: 2012-10-03 02:05 PDT (History)
8 users (show)

See Also:


Attachments
Patch (130.93 KB, patch)
2012-10-02 05:43 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Raphael Kubo da Costa (:rakuco) 2012-10-02 05:41:57 PDT
Remove comments from the Apache config files to start avoiding useless duplication.
Comment 1 Raphael Kubo da Costa (:rakuco) 2012-10-02 05:43:23 PDT
Created attachment 166678 [details]
Patch
Comment 2 Laszlo Gombos 2012-10-02 06:40:08 PDT
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 ?
Comment 3 Raphael Kubo da Costa (:rakuco) 2012-10-02 06:45:03 PDT
(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.
Comment 4 Laszlo Gombos 2012-10-02 08:44:38 PDT
Comment on attachment 166678 [details]
Patch

Looks good to me, r+.
Comment 5 Raphael Kubo da Costa (:rakuco) 2012-10-02 08:50:02 PDT
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.
Comment 6 Eric Seidel (no email) 2012-10-02 09:12:28 PDT
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.
Comment 7 Raphael Kubo da Costa (:rakuco) 2012-10-02 09:24:11 PDT
(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.
Comment 8 Eric Seidel (no email) 2012-10-02 09:39:13 PDT
SGTM.  I suspect each port requires very little custom config.
Comment 9 Raphael Kubo da Costa (:rakuco) 2012-10-02 09:51:33 PDT
Err, webkit-patch land-safely seems to have done only half of its job. I will land this manually.
Comment 10 Eric Seidel (no email) 2012-10-02 09:53:46 PDT
(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?
Comment 11 Raphael Kubo da Costa (:rakuco) 2012-10-02 09:57:10 PDT
Committed r130182: <http://trac.webkit.org/changeset/130182>
Comment 12 Raphael Kubo da Costa (:rakuco) 2012-10-02 10:01:07 PDT
(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.
Comment 13 Eric Seidel (no email) 2012-10-02 10:06:02 PDT
Still sounds like a bug.  It should probably abort earlier if there is no diff. :(
Comment 14 Eric Seidel (no email) 2012-10-02 10:07:13 PDT
(In reply to comment #13)
> Still sounds like a bug.  It should probably abort earlier if there is no diff. :(

Filed bug 98171.
Comment 15 Ojan Vafai 2012-10-02 11:48:18 PDT
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
Comment 16 Ojan Vafai 2012-10-02 11:49:11 PDT
Nm...I think the bots were just confused about revision numbers.
Comment 17 Alexey Proskuryakov 2012-10-02 15:38:30 PDT
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?
Comment 18 Eric Seidel (no email) 2012-10-02 16:01:02 PDT
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.
Comment 19 Eric Seidel (no email) 2012-10-02 16:02:45 PDT
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.)
Comment 20 Alexey Proskuryakov 2012-10-02 16:48:01 PDT
> 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.
Comment 21 Raphael Kubo da Costa (:rakuco) 2012-10-03 02:05:44 PDT
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.