make the build more strict with bad metadata.json files

RESOLVED FIXED

Status

Firefox OS
Gaia
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: julienw, Assigned: julienw)

Tracking

unspecified
x86_64
Linux

Firefox Tracking Flags

(blocking-b2g:-, b2g18+ fixed, b2g18-v1.0.1 fixed)

Details

(Whiteboard: u=fx-os-user c=may-6-17 p=1)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
We found that in some partner customizatons (eg [1]) the metadata.json files are wrong.

I'd like that we make the build stricter now so that we don't have strange builds later:

- make the manifestURL property mandatory, and fail if this is missing. (instead of having a magic concatenation)
- check that the origin is a real origin (ie: no path at the end). Investigate if we can infer this origin from the manifestURL.
- issue a warning when the manifestURL is an "app://" url, because this makes the packaged app unupdatable (which is sometimes desirable).

[1] https://github.com/telefonicaid/firefoxos-gaia-spain

Asking tef+ on this because while it's npotb I want that our partners are aware of this, and I want to prioritize this.
(Assignee)

Updated

5 years ago
blocking-b2g: --- → tef?
FWIW, I think this is a good idea to do for defect prevention, but I don't think a makefile protection in itself should block the release. But I definitely suggest going forward implementing this.

Updated

5 years ago
Blocks: 815411
thank you Julien for telling me about origin incorrect things. I also filed bugs on telefonica repositories and gaia-preload-app for incorrect origin.

https://github.com/yurenju/gaia-preload-app/issues/5
https://github.com/telefonicaid/firefoxos-gaia-spain/issues/2
https://github.com/telefonicaid/firefoxos-gaia-latam/issues/2

Updated

5 years ago
Blocks: 863032
No longer blocks: 815411
We won't block on this, but we'll track this so we get to this in future releases.
blocking-b2g: tef? → -
tracking-b2g18: --- → +
Fixed it on gaia-preload-app and I will make new version for telefonica repository on bug 864617.
(Assignee)

Updated

5 years ago
Assignee: nobody → felash
(Assignee)

Comment 5

5 years ago
another check we need to do :

* check that we have a manifest.webapp
should we check etag in metadata.json?
(In reply to Yuren Ju [:yurenju] from comment #6)
> should we check etag in metadata.json?

I don't think. We don't want to do network access at build time.
Whiteboard: u=fx-os-user c=may-6-17 p=0
Whiteboard: u=fx-os-user c=may-6-17 p=0 → u=fx-os-user c=may-6-17 p=1
(Assignee)

Comment 8

5 years ago
Created attachment 750509 [details] [diff] [review]
patch v1

* be stricter about metadata.json data
* fix the apps we have in the tree
---
 build/webapp-manifests.js                     |   91 +++++++++++++++++++------
 test_external_apps/hoststubtest/metadata.json |    2 +-
 test_external_apps/mochitest/metadata.json    |    3 +-
 3 files changed, 75 insertions(+), 21 deletions(-)
Attachment #750509 - Flags: review?(fabrice)
Comment on attachment 750509 [details] [diff] [review]
patch v1

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

Please use nsIURI for all the uri checks instead of string based manipulation.
Attachment #750509 - Flags: review?(fabrice) → review-
(Assignee)

Comment 10

5 years ago
Created attachment 751059 [details] [diff] [review]
patch v2

* be stricter about metadata.json data
* fix the apps we have in the tree
---
 build/webapp-manifests.js                     |   95 ++++++++++++++++++++-----
 test_external_apps/hoststubtest/metadata.json |    2 +-
 test_external_apps/mochitest/metadata.json    |    3 +-
 3 files changed, 79 insertions(+), 21 deletions(-)

Now uses nsIURI, thanks for the great suggestion.
Attachment #750509 - Attachment is obsolete: true
Attachment #751059 - Flags: review?(fabrice)
Comment on attachment 751059 [details] [diff] [review]
patch v2

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

r=me with nits fixed.

::: build/webapp-manifests.js
@@ +4,5 @@
>    //dump('-*- webapp-manifest.js: ' + msg + '\n');
>  }
>  
> +let io = Components.classes['@mozilla.org/network/io-service;1']
> +                .getService(Components.interfaces.nsIIOService);

nit: s/Components.classes/Cc, and align .getService with ['@mozilla.org/network...

@@ +135,5 @@
> +                'mandatory manifestURL property.');
> +    return;
> +  }
> +
> +  let manifestURLURI;

very weird variable name. What about manifestURI ?
Comment on attachment 751059 [details] [diff] [review]
patch v2

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

r=me with nits fixed.

::: build/webapp-manifests.js
@@ +4,5 @@
>    //dump('-*- webapp-manifest.js: ' + msg + '\n');
>  }
>  
> +let io = Components.classes['@mozilla.org/network/io-service;1']
> +                .getService(Components.interfaces.nsIIOService);

nit: s/Components.classes/Cc, and align .getService with ['@mozilla.org/network...

@@ +135,5 @@
> +                'mandatory manifestURL property.');
> +    return;
> +  }
> +
> +  let manifestURLURI;

very weird variable name. What about manifestURI ?
Attachment #751059 - Flags: review?(fabrice) → review+
(Assignee)

Comment 13

5 years ago
Created attachment 751347 [details] [diff] [review]
patch v3

fixed nits, carrying r=fabrice
PR in https://github.com/mozilla-b2g/gaia/pull/9862 to see if Travis is happy
Attachment #751059 - Attachment is obsolete: true
Attachment #751347 - Flags: review+
(Assignee)

Comment 14

5 years ago
master: 7d8f462d9402c306fa243c96c3d660a399128acc
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 15

5 years ago
a=npotb
v1-train: 9380ceb81b3eac45861b8d1be07ab7f748ed52a3
v1.0.1: 62a6ed13e3e135e7183a1af2eef3b55f5ffb0378
status-b2g18: --- → fixed
status-b2g18-v1.0.1: --- → fixed

Updated

5 years ago
No longer blocks: 863032
You need to log in before you can comment on or make changes to this bug.