1079304 - Remove filesize from Firefox & Fennec json files

Status: RESOLVED WONTFIX

(www.mozilla.org :: Product Details, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

Remove the filesize. As seen in bug 1070004, the declaration is no longer used.

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Attachment: remove-filesize.diff

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Attachment: json-without-filesize.diff

FYI, the diff without the filesize.

2K => 221
116K => 11K

Comment 3

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8501105 [details] [diff] [review]
remove-filesize.diff

Looks good to me. Let's see if this breaks anything downstream.

Comment 4

[Sylvestre Ledru [:Sylvestre]]

Merged. 
export_json:
http://viewvc.svn.mozilla.org/vc?view=revision&revision=132677

json files regenerated:
http://viewvc.svn.mozilla.org/vc?view=revision&revision=132678

Comment 5

[Paul [:pmac] McLanahan]

Broke a bit of everything downstream :( We're reverting r132678. Removing "filesize" is fine, completely changing the data-structure not so much.

Comment 6

[Pascal Chevrel:pascalc (PTO until April 26)]

I reverted the regeneration of json files per the webdev team request as this patch breaks www.mozilla.org and prevents pushes to production today. (svn merge -c -132678 .)

Comment 7

[Pascal Chevrel:pascalc (PTO until April 26)]

also reverted code change in r132689 to make sure the export script causing the failure is not ran by mistake until we have a final fix.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Sorry, my bad. I was too aggressive in my change. Sorry :/

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Attachment: remove_filesize_2.diff

Is that OK with you? New JSON is the next attachment.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Attachment: remove_filesize_example_2.diff

Comment 11

[Paul [:pmac] McLanahan]

Comment on attachment 8501586 [details] [diff] [review]
remove_filesize_example_2.diff

Review of attachment 8501586 [details] [diff] [review]:
-----------------------------------------------------------------

This still changes the data type of the builds. I don't disagree with even your first attempt at a new structure, but it's very hard to say how people are using the data. Our python library just loads the JSON, which the app is then able to query however they like, and if they're expecting a dict and find a list (which is what broke) then it can throw unexpected errors. The safest thing would be to just have an empty object. e.g.:

{
    "ku": {
        "33.0b8": {
            "Windows": {},
            "OS X": {},
            "Linux": {}
        },
        "34.0a2": {
            "Windows": {},
            "OS X": {},
            "Linux": {}
        }
    },
    ...
}

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Attachment: remove_filesize_example_3.diff

better ?

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Attachment: remove-filesize_3.diff

Comment 14

[Paul [:pmac] McLanahan]

Comment on attachment 8501741 [details] [diff] [review]
remove_filesize_example_3.diff

Review of attachment 8501741 [details] [diff] [review]:
-----------------------------------------------------------------

Hmm... Better, yes. I'm not sure that's enough though. There could easily be sites expecting an object instead of a string as the value of these OS entries. What do you think :jgmize?

Comment 15

[Josh Mize [:jgmize]]

(In reply to Paul McLanahan [:pmac] from comment #14)
> Hmm... Better, yes. I'm not sure that's enough though. There could easily be
> sites expecting an object instead of a string as the value of these OS
> entries. What do you think :jgmize?

Yes, I agree with the conservative approach of not changing the data structure other than removing the unused field. If more radical changes are needed/desired, it would probably be best to introduce some sort of version scheme, for example by adding a "v2" directory with new files with arbitrary changes to the data structures that don't need to maintain backwards compatibility, but still preserving the current data structures in the existing files.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

I understand your points but the goal of the work we are doing on product details is to remove as much legacy as possible.

For example, the list
            "Windows": "",
            "OS X": "",
            "Linux": ""
is probably useless. AFAIK, for every locales and versions, we ship on the 3 OS.

I would like to get ride of that too. So, if you think that a v2 is the only way to achieve that, why not!

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Attachment: side-effect.diff

FYI, in order to have 
"Windows": {},
instead of:
"Windows": "",

The patch is:
Index: export_json.php
===================================================================
--- export_json.php	(révision 132736)
+++ export_json.php	(copie de travail)
@@ -21,7 +21,7 @@
  */
 function writefile($filename, $data)
 {
-    file_put_contents(JSONDIR.$filename, json_encode($data, JSON_PRETTY_PRINT));
+    file_put_contents(JSONDIR.$filename, json_encode($data, JSON_PRETTY_PRINT | JSON_FORCE_OBJECT));
 }
 
 /** Locale Details */


but it has side effects. It is changing json/mobile_details.json in a way which could affect others.

Comment 18

[Paul [:pmac] McLanahan]

Comment on attachment 8502403 [details] [diff] [review]
side-effect.diff

Review of attachment 8502403 [details] [diff] [review]:
-----------------------------------------------------------------

I don't see the point of changing the data structure away from an array here.

Comment 19

[Paul [:pmac] McLanahan]

I see you say that the above is a side effect, hmm..... Perhaps it would be best to just leave this alone and go with a v2 for now, with the understanding that we'll be working to get a better system in place with releng (the ship-it based thing)?

Comment 20

[Paul [:pmac] McLanahan]

Comment on attachment 8501741 [details] [diff] [review]
remove_filesize_example_3.diff

Review of attachment 8501741 [details] [diff] [review]:
-----------------------------------------------------------------

Still balking at this change. Still agreed that filesize is superfluous, but I do think the data-structures in these files need to remain constant for now.

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Attachment: export_v2.diff

With a new schema (but keep the previous one)

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Attachment: v2_without_filesize_example_4.diff

The example of a v2 schema

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Here it is Paul.

By the way, do you use json/firefox_beta_builds.json?

Comment 24

[Paul [:pmac] McLanahan]

Yes we do. Our /{product_name}/all/ pages list "beta" locales, which come from that file.

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Touching product details is too risky. We will work on bug 1083718 instead.

Comment 26

[Paul [:pmac] McLanahan]

Comment on attachment 8508535 [details] [diff] [review]
export_v2.diff

Review of attachment 8508535 [details] [diff] [review]:
-----------------------------------------------------------------

::: export_json.php
@@ +95,5 @@
>      writefile("firefox_{$build}.json", $buildArray);
> +
> +    // New schema (to remove the filesize)
> +    $buildArray = fillFileSize($fxd->$build, true);
> +    writefile("firefox_{$build}_v2.json", $buildArray);

Probably better if it output to a separate directory called "v2" to keep it cleanly separate.