Closed Bug 957497 Opened 10 years ago Closed 10 years ago

Allow include of uncompressed data files in the package apps

Categories

(Firefox OS Graveyard :: Gaia, defect)

Product:
Firefox OS Graveyard
Component:
Gaia
Platform:
All
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-b2g:1.3T+, b2g-v1.3T fixed, b2g-v1.4 fixed)

Status:
RESOLVED FIXED
Project Flags:
blocking-b2g 1.3T+
Tracking Flags:
Tracking Status
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: timdream, Assigned: yurenju)

References

Details

Attachments

(3 files, 2 obsolete files)

example for system app
102 bytes, application/json
Details
github PR: https://github.com/mozilla-b2g/gaia/pull/15961
46 bytes, text/x-github-pull-request
timdream
: review+
Details | Review
point to https://github.com/mozilla-b2g/gaia/pull/17218
46 bytes, text/x-github-pull-request
yurenju
: review+
Details | Review 
+++ This bug was initially created as a clone of Bug #945152 +++

Bug 945152 will introduce a feature allowing TypedArrays to be backed by file in the disk instead of a block of memory. We should change Gaia build system accordingly to put these files in the zip uncompressed so we could benefit from it (although according to Bug 945152 comment 37 the file will be extracted by Gecko too if included compressed)

Please implement the feature in build script so that

-- Apps could explicitly specify which files should be included in the zip uncompressed (no more regexp on file name magic, please)
-- This feature can be turned off altogether with a pref (if a certain vendor thinks ROM space is more valuable than memory).


Thanks!
Flags: needinfo?(yurenju.mozilla)
Given that we also have restrictions on the /system partition size, we need to be careful with what we do there.
will Yuren be able to take this bug? thanks
sure.
Assignee: nobody → yurenju.mozilla
Flags: needinfo?(yurenju.mozilla)
I would like to introduce a build.config in app directory with below format:

> {
>   "zip": {
>     "COMPRESSION_NONE": [
>       "filepath-without-slash-prefix",
>       "files-in-shared-also-need-to-add",
>       "resources/icc.json",
>       "shared/resources/languages.json"
>     ] 
>   }
> }

and we can read those configration in webapp-zip.js for each app.

Fabrice & Tim, does it make sense?
Flags: needinfo?(timdream)
Flags: needinfo?(fabrice)
Discussed offline. We should use |metadata.json| instead of creating another JSON config file that doesn't even end with |.json|. The way we handle external-apps needs to be changed too because the current build script thinks the existence of such file makes the app an external app.

So:

metadata.json

{
  external: false,
  zip: {
    no_compression: [
      ...
    ]
  }
}

PS please keep everything lower case and underlined.
Flags: needinfo?(timdream)
Per bug 945152 comment 47, the file stored in zip package need to be page aligned for ArrayBuffer.
I think we need to modify zip tool to achieve this.
That looks a lot more complicated now... I'm wondering if we should not do something drastically different for that app:// protocol instead. I'll run some experiments and come back with results.
Flags: needinfo?(fabrice)
(In reply to Shian-Yow Wu [:shianyow] from comment #6)
> Per bug 945152 comment 47, the file stored in zip package need to be page
> aligned for ArrayBuffer.
> I think we need to modify zip tool to achieve this.

I filed bug 959047 for this.
Depends on: 959047
No longer depends on: 945152
Depends on: 945152
Given the new alignment requirement, maybe s/no_compression/store_and_align/g in my comment 5 proposal?
This is the part 1 pull request for specifying files to add into zip without compression.

part 2 pull request for page aligned for ArrayBuffer should be a one line patch to call zip.alignStoredFiles(4096).
Attachment #8361583 - Flags: review?(timdream)
BTW, webapp-zip.js should be refactoring since there are too much rules in the code, I wrote a document to figure out how webapp-zip.js works, but this issue is critual so I'll file another bug for it.
Tim discussed with me about the integration test because the test case will be failed if anyone touch something in system which does not make sense.

I'll remove it on next pull reqeust.
I have removed the integration test for webapp-zip.js.
Comment on attachment 8361583 [details] [review]
github PR: https://github.com/mozilla-b2g/gaia/pull/15450

Looks good. If you make and modification based on my suggestions, please make sure you have test the new patch thoroughly. Thank you!
Attachment #8361583 - Flags: review?(timdream) → review+
Discussed with Tim offline for naming that property, there are some canidates:
1. no_compression: missing align meaning
2. store_and_align: "stored" is the name of compression method without any compession[1]
3. no_compression_and_align: too long
4. mmap_files: shorter and more explicit for implmentation details.

so we decided to use mmap_files.

[1] https://en.wikipedia.org/wiki/Zip_%28file_format%29#Compression_methods
unzip -v profile/webapps/system.gaiamobile.org/application.zip

> Archive:  profile/webapps/system.gaiamobile.org/application.zip
>  Length   Method    Size  Ratio   Date   Time   CRC-32    Name
> --------  ------  ------- -----   ----   ----   ------    ----
> ...
>    17084  Stored    17084   0%  01-01-70 08:00  057d826b  resources/sounds/unlock.ogg
> ...
some integration test failed, I'm investigating...
The problem is properties files are concated into locales-obj/ and no longer exists in zip, we need modify test cases to access /loacles-obj/en-US.json instead of /locales/matcher.en-US.properties.
merged.

https://github.com/mozilla-b2g/gaia/commit/fd157cf37c851fdbd97075f414ea07e8e8f28b52

I would not close this bug because we're still waiting gecko patch for page align on bug 959047 for now. once gecko patch is landed I'll send a one line patch for enabling page align.
feaure detect for nsIZipWriter.alignStoredFiles()
Attachment #8362771 - Flags: review?(timdream)
Comment on attachment 8362771 [details] [review]
github PR: https://github.com/mozilla-b2g/gaia/pull/15539

Ouch, I didn't realize you were intend to merge two patch separately on the same bug. We should never do that unless it's absolute necessary to make sure the bug status is either fixed with merged patch or unfixed with no patch merged.

That said, I guess this one is still safe enough. Please don't do that next time.
Attachment #8362771 - Flags: review?(timdream) → review+
This commit has caused error when trying to |make install-gaia APP=XXX| on Windows.
All the filenames in the zip has prefix '\' , I guess it's the problem...

https://github.com/yurenju/gaia/commit/3eb52bf51d1ce57cdbbf9ee8cade488d77985ef1
Flags: needinfo?(yurenju.mozilla)
Tim, got it and sorry for that, I won't do it again.

George, I filed bug 962938 for it.


since part 1 pr was landed, I'll land part pull request and solve the problem on bug 962938.
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: needinfo?(yurenju.mozilla)
Resolution: --- → FIXED
Yuren,

1. Does this patch automatically flags locales-obj/{{lang}}.data as mmap_files? (the files we are going to create in bug 958440). If not please file a follow-up bug or ask Evelyn to do so in her bug.
2. The patches here did not change/create any of the metadata.json. Does that means we are now recognizing marketplace app as non-external app?
3. I need to create another bug to create metadata.json for IME data files right?
Flags: needinfo?(yurenju.mozilla)
1. no and bug 962983 is filed
2. no, we don't, if an app doesn't contain metadata.json, we assume it's a non-external app. if contains a metadata.json but there is no "external" property, we assume it's an external app, we have a function in |build/utils.js@isExternalApp()|
3. yes
Flags: needinfo?(yurenju.mozilla)
(In reply to Yuren Ju [:yurenju][:小朱] from comment #29)
> 2. no, we don't, if an app doesn't contain metadata.json, we assume it's a
> non-external app. if contains a metadata.json but there is no "external"
> property, we assume it's an external app, we have a function in
> |build/utils.js@isExternalApp()|

Sorry for not getting this in review, but I would say the rule here is a bit of confusing. An app dir gets to be recognized as internal by default, external if comes with metadata.json, and internal again if you explicitly set external:false in metadata?

https://github.com/mozilla-b2g/gaia/commit/fd157cf37c851fdbd97075f414ea07e8e8f28b52#diff-2b31def028024b968babe1d78830dfafR44

We should employ easier logic here unless you want to keep the backward comparability (to out-of-tree apps maybe?)
Flags: needinfo?(yurenju.mozilla)
I filed bug 963463 for it.
Flags: needinfo?(yurenju.mozilla)
See Also: → 963463
Travis-ci: https://travis-ci.org/mozilla-b2g/gaia/builds/18254658

root cause is |addToZip()| is no longer support add a directory, so we need to use utils.ls(FILE, true) to get all files in directory.

the major changes is webapp-zip.js:L584-L592
https://github.com/mozilla-b2g/gaia/pull/15961/files#diff-8d06cf2fa60a709d7fe1324030517ce8R584


Tim, could you review it again? thank you.
Attachment #8361583 - Attachment is obsolete: true
Attachment #8362771 - Attachment is obsolete: true
Attachment #8370601 - Flags: review?(timdream)
Comment on attachment 8370601 [details] [review]
github PR: https://github.com/mozilla-b2g/gaia/pull/15961

Look like we have a conflict here.
Attachment #8370601 - Flags: review?(timdream) → review+
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
nominate 1.3T? because of bug 945152, we need the patch here to make bug 945152 work.
blocking-b2g: --- → 1.3T?
1.3T?, [remove] tarako
Whiteboard: [tarako]
hi Evelyn, during the triage, we talked about doing further test on this. do we still need this for tarako? Thanks
Flags: needinfo?(ehung)
yeah, I did the test but can't tell anything better... I need someone to further evaluate the result. Will ping :shianyow to see if any tool can produce a fair report.
Flags: needinfo?(ehung)
ping again to see if we have further updates. thanks
Flags: needinfo?(ehung)
Tested but found some problems, waiting :shianyow's further fix on bug 945152.
Flags: needinfo?(ehung)
945152 is close to getting fixed and this will be needed, hence blocking
blocking-b2g: 1.3T? → 1.3T+
Hi Ying Xu, heard that you will be doing uplifts to 1.3T branch. After you completed the uplift, can you please set status-b2g-v1.3T to fixed? please let us know if you have problems with it. thanks
Flags: needinfo?(ying.xu)
Hi,yuren.

Would you please rebase your patch on v1.3t branch?
There're some huge conflicts I can not handle them.

# 未合并的路径：
#   （酌情使用 "git add/rm <file>..." 标记解决方案）
#
#	由我们删除：  build/multilocale.js
#	由我们删除：  build/test/integration/multilocale.test.js
#	由我们删除：  build/utils-xpc.js
#	双方修改：     build/utils.js
#	双方修改：     build/webapp-zip.js
Flags: needinfo?(ying.xu) → needinfo?(yurenju.mozilla)
talked to evelyn and she has a pull request for 1.3t.
Flags: needinfo?(yurenju.mozilla)
needinfo evelyn.
Flags: needinfo?(ehung)
This patch is for 1.3t. Since build script in between master and 1.3t differs a lot, ask for review again.
Attachment #8391815 - Flags: review?(yurenju.mozilla)
Flags: needinfo?(ehung)
See Also: → 984212
Comment on attachment 8391815 [details] [review]
point to https://github.com/mozilla-b2g/gaia/pull/17218

looks good to me but we should have test case for it, filed follow up bug 984212.
Attachment #8391815 - Flags: review?(yurenju.mozilla) → review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: