Bug 68999 – Web Inspector: make inspector protocol validation a part of the build process.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Pavel Feldman]

Since we'd like to commit to inspector protocol backwards compatibility, violating it should be a build failure.

[Pavel Feldman]

Created attachment 109017 [details]
Patch

[Yury Semikhatsky]

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"

[Joseph Pecoraro]

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

[Pavel Feldman]

> 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!

[Pavel Feldman]

Created attachment 109032 [details]
[Patch] spaces -> tab for makefiles.

[Pavel Feldman]

Committed r96320: <http://trac.webkit.org/changeset/96320>

[David Levin]

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.

[Pavel Feldman]

(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.

[David Levin]

(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. :)