Bug 68999

Summary: Web Inspector: make inspector protocol validation a part of the build process.
Product: WebKit Reporter: Pavel Feldman <pfeldman>
Component: Web Inspector (Deprecated)Assignee: Pavel Feldman <pfeldman>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, joepeck, keishi, levin, loislo, pfeldman, pmuellr, rik, timothy, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
[Patch] spaces -> tab for makefiles. none

Description Pavel Feldman 2011-09-28 06:19:36 PDT
Since we'd like to commit to inspector protocol backwards compatibility, violating it should be a build failure.
Comment 1 Pavel Feldman 2011-09-28 06:24:40 PDT
Created attachment 109017 [details]
Patch
Comment 2 Yury Semikhatsky 2011-09-28 07:21:13 PDT
Comment on attachment 109017 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109017&action=review

> Source/WebCore/inspector/validate-protocol-compatibility:308
> +        sys.stderr.write("Usage: %s -o OUTPUT_FILE INPUT_FILE\n" % sys.argv[0])

Since there are two required parameters and their positions are fixed, let's omit "-o"
Comment 3 Joseph Pecoraro 2011-09-28 10:02:26 PDT
Comment on attachment 109017 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109017&action=review

> Source/WebCore/DerivedSources.make:895
> +Inspector.json.validated : Inspector.json inspector/validate-protocol-compatibility
> +        python $(WebCore)/inspector/validate-protocol-compatibility -o Inspector.json.validated $(WebCore)/inspector/Inspector.json
> +
>  Inspector.idl : Inspector.json inspector/generate-inspector-idl
>  	python $(WebCore)/inspector/generate-inspector-idl -o Inspector.idl $(WebCore)/inspector/Inspector.json

Do make files require Tabs? I wonder if the 8 spaces here will cause an issue. The Inspector.idl generator example below appears to use a tab.

> Source/WebCore/GNUmakefile.am:691
> +DerivedSources/WebCore/Inspector.json.validated : $(WebCore)/inspector/Inspector.json $(WebCore)/inspector/validate-protocol-compatibility
> +        $(PYTHON) $(WebCore)/inspector/validate-protocol-compatibility -o $(GENSOURCES_WEBCORE)/Inspector.json.validated $(WebCore)/inspector/Inspector.json

ditto
Comment 4 Pavel Feldman 2011-09-28 10:12:22 PDT
> Do make files require Tabs? I wonder if the 8 spaces here will cause an issue. The Inspector.idl generator example below appears to use a tab.
> 
> > Source/WebCore/GNUmakefile.am:691
> > +DerivedSources/WebCore/Inspector.json.validated : $(WebCore)/inspector/Inspector.json $(WebCore)/inspector/validate-protocol-compatibility
> > +        $(PYTHON) $(WebCore)/inspector/validate-protocol-compatibility -o $(GENSOURCES_WEBCORE)/Inspector.json.validated $(WebCore)/inspector/Inspector.json
> 
> ditto

I'll fix those, thanks for the heads up!
Comment 5 Pavel Feldman 2011-09-28 10:16:01 PDT
Created attachment 109032 [details]
[Patch] spaces -> tab for makefiles.
Comment 6 Pavel Feldman 2011-09-29 04:49:03 PDT
Committed r96320: <http://trac.webkit.org/changeset/96320>
Comment 7 David Levin 2011-09-29 14:08:14 PDT
Why is this a build failure as opposed to a test failure?

Putting something in the build slows down a process (the build -- I may do several builds before I get everything ready to run the test cases -- also my machine is under more stress when I do a build, etc.) that is run more often than tests.
Comment 8 Pavel Feldman 2011-09-29 22:00:14 PDT
(In reply to comment #7)
> Why is this a build failure as opposed to a test failure?
> 
> Putting something in the build slows down a process (the build -- I may do several builds before I get everything ready to run the test cases -- also my machine is under more stress when I do a build, etc.) that is run more often than tests.

I have to admit that initially it was more of a paranoia on our side: we wanted this check to never be suppressed. However, now there is a patch pending that converts this validation into a header generator (https://bugs.webkit.org/show_bug.cgi?id=69092). So now this generation piece needs to be a part of the build.

Talking about the several builds, new target is a quick python check that depends on two files: inspector protocol schema and the python script itself. I would expect it to trigger only once for you (unless you are modifying these files). Please tell me if this is not the case.

Broader context:
1) we are consolidating all the generation steps in the inspector into a single python-based one, getting rid of the intermediate IDL step that is using bindings' perl script
2) once (1) is complete, we will add runtime traffic validation while in the debug mode.
3) once (2) is ready, we will consider migrating to the layouttest-based validation.
Comment 9 David Levin 2011-09-29 22:05:56 PDT
(In reply to comment #8)
> (In reply to comment #7)
> > Why is this a build failure as opposed to a test failure?
> > 
> > Putting something in the build slows down a process (the build -- I may do several builds before I get everything ready to run the test cases -- also my machine is under more stress when I do a build, etc.) that is run more often than tests.
> 
> I have to admit that initially it was more of a paranoia on our side: we wanted this check to never be suppressed. However, now there is a patch pending that converts this validation into a header generator (https://bugs.webkit.org/show_bug.cgi?id=69092). So now this generation piece needs to be a part of the build.
> 
> Talking about the several builds, new target is a quick python check that depends on two files: inspector protocol schema and the python script itself. I would expect it to trigger only once for you (unless you are modifying these files). Please tell me if this is not the case.
> 
> Broader context:
> 1) we are consolidating all the generation steps in the inspector into a single python-based one, getting rid of the intermediate IDL step that is using bindings' perl script
> 2) once (1) is complete, we will add runtime traffic validation while in the debug mode.
> 3) once (2) is ready, we will consider migrating to the layouttest-based validation.

Thanks for the info. I just saw this and got a bit paranoid about unnecessary things being added to the build. :)