Bug 224561

Summary: [LayoutTests] Decouple http/tests/media/resources/create-id3-db.php from webserver
Product: WebKit Reporter: Chris Gambrell <cgambrell>
Component: Tools / TestsAssignee: Chris Gambrell <cgambrell>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, cgambrell, darin, eric.carlson, ews-watchlist, glenn, jbedard, jer.noble, philipj, pnormand, ryanhaddad, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=224528
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Gambrell 2021-04-14 10:34:49 PDT
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.
Comment 1 Radar WebKit Bug Importer 2021-04-14 10:36:04 PDT
<rdar://problem/76653054>
Comment 2 Philippe Normand 2021-04-15 04:13:43 PDT
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.
Comment 3 Philippe Normand 2021-04-15 04:16:39 PDT
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.
Comment 4 Chris Gambrell 2021-05-03 11:00:34 PDT
Created attachment 427576 [details]
Patch
Comment 5 Jonathan Bedard 2021-05-03 11:03:05 PDT
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
Comment 6 Chris Gambrell 2021-05-03 11:10:26 PDT
Created attachment 427577 [details]
Patch
Comment 7 Chris Gambrell 2021-05-03 11:11:14 PDT
> > 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
Comment 8 Jonathan Bedard 2021-05-05 16:02:41 PDT
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
Comment 9 Eric Carlson 2021-05-05 16:33:03 PDT
(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?
Comment 10 Darin Adler 2021-05-09 15:45:38 PDT
Comment on attachment 427577 [details]
Patch

Oops, clearing the flags.
Comment 11 Darin Adler 2021-05-09 15:46:24 PDT
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.
Comment 12 Jonathan Bedard 2021-05-10 07:44:50 PDT
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.
Comment 13 Chris Gambrell 2021-05-10 08:56:05 PDT
Created attachment 428175 [details]
Patch
Comment 14 Chris Gambrell 2021-05-10 09:02:06 PDT
Created attachment 428176 [details]
Patch
Comment 15 Jonathan Bedard 2021-05-10 09:18:32 PDT
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
Comment 16 Chris Gambrell 2021-05-10 10:57:50 PDT
Created attachment 428183 [details]
Patch
Comment 17 Chris Gambrell 2021-05-10 10:59:30 PDT
(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
Comment 18 Jonathan Bedard 2021-05-10 11:37:40 PDT
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
Comment 19 Chris Gambrell 2021-05-10 11:41:14 PDT
Created attachment 428189 [details]
Patch
Comment 20 Chris Gambrell 2021-05-10 11:44:36 PDT
(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
Comment 21 EWS 2021-05-10 17:41:57 PDT
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].