Bug 224561 – [LayoutTests] Decouple http/tests/media/resources/create-id3-db.php from webserver

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Chris Gambrell]

WebKit is deprecating PHP support for layout tests, and contributors are not expected to have PHP configured to work on the project in the coming weeks. We should replace create-id3-db.php with a supported language, perhaps Python or Perl?

LayoutTests/media/content/metadata.db is being converted to and read in JSON as opposed to PHP serialized in https://bugs.webkit.org/show_bug.cgi?id=224528.

[Radar WebKit Bug Importer]

<rdar://problem/76653054>

[Philippe Normand]

This script was added as part of fixing bug 125926.
Eric Carlson's worry, IIUC, is that adding a binary file without anyway to regenerate it, was an issue.

Since the db is now in json format, I think this worry is less concerning perhaps? The create-id3-db script could be ported to Python still, but I'd say it's not high priority.

[Philippe Normand]

I've checked http/tests/media/media-play-stream-chunked-icy.html still pass'es as expected on GTK with the patch from bug 224528 applied.

[Chris Gambrell]

Created attachment 427576 [details]
Patch

[Jonathan Bedard]

Comment on attachment 427576 [details]
Patch

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

> LayoutTests/ChangeLog:3
> +        [LayoutTests] Replace http/tests/media/resources/create-id3-db.php with supported language

Worth changing the title of this bug to "Remove"

> LayoutTests/ChangeLog:8
> +

Should probably include the rationale for removing, we can generally expect folks to be able to understand and modify JSON

[Chris Gambrell]

Created attachment 427577 [details]
Patch

[Chris Gambrell]

> > LayoutTests/ChangeLog:8
> > +
> 
> Should probably include the rationale for removing, we can generally expect
> folks to be able to understand and modify JSON

Addressed in comment 6

[Jonathan Bedard]

The replacement for the the PHP database is LayoutTests/media/content/metadata.json, I'd like someone more familiar with our media tests to verify that this is the right approach, the problem I see is that although the metadata.json is certainly easier to edit than it's PHP counterpart, the contents of the file still aren't trivial to regenerate

[Eric Carlson]

(In reply to Jonathan Bedard from comment #8)
> The replacement for the the PHP database is
> LayoutTests/media/content/metadata.json, I'd like someone more familiar with
> our media tests to verify that this is the right approach, the problem I see
> is that although the metadata.json is certainly easier to edit than it's PHP
> counterpart, the contents of the file still aren't trivial to regenerate

The old database was generated by the php. If metadata.json is generated by the python we should be OK. If it isn't, why not?

[Darin Adler]

Comment on attachment 427577 [details]
Patch

Oops, clearing the flags.

[Darin Adler]

I agree with Eric. We should replace these scripts with the new scripts to generate the checked-in JSON file. Not remove them with no replacement.

Sorry, I didn’t mean to clear the flags, but once I had set review+ and commit-queue+ that was the only way I knew to undo it.

[Jonathan Bedard]

Talking with Alexey and Chris about this, the plan is to change this script to a stand-alone PHP script so we can pull PHP out of our web servers.

[Chris Gambrell]

Created attachment 428175 [details]
Patch

[Chris Gambrell]

Created attachment 428176 [details]
Patch

[Jonathan Bedard]

Comment on attachment 428176 [details]
Patch

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

> LayoutTests/media/content/create-id3-db.php:1
> +#!/usr/bin/env php

We don't need the ".php" extension any more. It's actually undesirable since it risks counting this file as a test if things get moved around. Just drop the .php and make the file executable.

> LayoutTests/media/content/create-id3-db.php:17
>      // $ Tools/Scripts/run-webkit-httpd

Change the running instructions, this should no longer involve the httpd server.

> LayoutTests/media/content/create-id3-db.php:31
> +    if (!function_exists('sys_get_temp_dir')) {

I don't think we need this check, since we aren't importing the file any more.

> LayoutTests/media/content/create-id3-db.php:51
> +    if (!function_exists('file_put_contents')) {

I don't think we need this check, since we aren't importing the file any more.

> LayoutTests/media/content/create-id3-db.php:65
> +    if (!function_exists('stream_copy_to_stream')) {

I don't think we need this check, since we aren't importing the file any more.

> LayoutTests/media/content/create-id3-db.php:80
> +    if (!function_exists('scandir')) {

I don't think we need this check, since we aren't importing the file any more.

> LayoutTests/media/content/create-id3-db.php:168
> +    }

We only ported

[Chris Gambrell]

Created attachment 428183 [details]
Patch

[Chris Gambrell]

(In reply to Jonathan Bedard from comment #15)
> Comment on attachment 428176 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=428176&action=review
> 
> > LayoutTests/media/content/create-id3-db.php:1
> > +#!/usr/bin/env php
> 
> We don't need the ".php" extension any more. It's actually undesirable since
> it risks counting this file as a test if things get moved around. Just drop
> the .php and make the file executable.
> 
> > LayoutTests/media/content/create-id3-db.php:17
> >      // $ Tools/Scripts/run-webkit-httpd
> 
> Change the running instructions, this should no longer involve the httpd
> server.
> 
> > LayoutTests/media/content/create-id3-db.php:31
> > +    if (!function_exists('sys_get_temp_dir')) {
> 
> I don't think we need this check, since we aren't importing the file any
> more.
> 
> > LayoutTests/media/content/create-id3-db.php:51
> > +    if (!function_exists('file_put_contents')) {
> 
> I don't think we need this check, since we aren't importing the file any
> more.
> 
> > LayoutTests/media/content/create-id3-db.php:65
> > +    if (!function_exists('stream_copy_to_stream')) {
> 
> I don't think we need this check, since we aren't importing the file any
> more.
> 
> > LayoutTests/media/content/create-id3-db.php:80
> > +    if (!function_exists('scandir')) {
> 
> I don't think we need this check, since we aren't importing the file any
> more.
> 
> > LayoutTests/media/content/create-id3-db.php:168
> > +    }
> 
> We only ported

Fixed in comment 16

[Jonathan Bedard]

Comment on attachment 428183 [details]
Patch

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

> LayoutTests/ChangeLog:3
> +        [LayoutTests] Replace http/tests/media/resources/create-id3-db.php result with JSON

We should change the title of this bug, something to the effect of "decouple create-id3-db from webserver"

> LayoutTests/ChangeLog:9
> +        Reviewed by NOBODY (OOPS!).

Reviewed by line needs to go above the change explanation

[Chris Gambrell]

Created attachment 428189 [details]
Patch

[Chris Gambrell]

(In reply to Jonathan Bedard from comment #18)
> Comment on attachment 428183 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=428183&action=review
> 
> > LayoutTests/ChangeLog:3
> > +        [LayoutTests] Replace http/tests/media/resources/create-id3-db.php result with JSON
> 
> We should change the title of this bug, something to the effect of "decouple
> create-id3-db from webserver"
> 
> > LayoutTests/ChangeLog:9
> > +        Reviewed by NOBODY (OOPS!).
> 
> Reviewed by line needs to go above the change explanation

Fixed in comment 19

[EWS]

Committed r277311 (237571@main): <https://commits.webkit.org/237571@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 428189 [details].