Closed Bug 957241 Opened 6 years ago Closed 6 years ago

Don't package the full sdk when we don't need it

Categories

(Firefox OS Graveyard :: Runtime, defect, P1)

All
Gonk (Firefox OS)
defect

Tracking

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

RESOLVED FIXED
2.0 S1 (9may)
blocking-b2g 1.3T+
Tracking Status
b2g-v1.3T --- fixed

People

(Reporter: fabrice, Assigned: kanru)

References

Details

Attachments

(1 file)

In b2g many parts of the Addon sdk are useless, but copy_source.py generates a makefile that leads to everything being shipped.

We need to make copy_source.py able to filter out some of the directories/files in there. For instance, we don't need tests/, places/, tabs/, ui/, window/ on b2g.
Flags: needinfo?(dtownsend+bugmail)
Attached patch sdk-b2g.patchSplinter Review
That works here for b2g, I'll push to try to make sure I'm no breaking the world.
Attachment #8356744 - Flags: feedback?(dtownsend+bugmail)
Blocks: 956982
Comment on attachment 8356744 [details] [diff] [review]
sdk-b2g.patch

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

Looks fine to me, it will probably need a build config peer to review though.
Attachment #8356744 - Flags: feedback?(dtownsend+bugmail) → feedback+
Flags: needinfo?(dtownsend+bugmail)
Attachment #8356744 - Flags: review?(gps)
Priority: -- → P1
Comment on attachment 8356744 [details] [diff] [review]
sdk-b2g.patch

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

FWIW, copy_source.py will likely get rolled into an install manifest at some point. Don't be surprised when the build module comes knocking and rewrites this.
Attachment #8356744 - Flags: review?(gps) → review+
On several of the mochitest-bc jobs on this push, this failure showed up:
https://tbpl.mozilla.org/php/getParsedLog.php?id=32796690&tree=B2g-Inbound

So I backed out the patch from this bug in https://hg.mozilla.org/integration/b2g-inbound/rev/623b7361abfa
(In reply to Wes Kocher (:KWierso) from comment #5)
> On several of the mochitest-bc jobs on this push, this failure showed up:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=32796690&tree=B2g-Inbound
> 
> So I backed out the patch from this bug in
> https://hg.mozilla.org/integration/b2g-inbound/rev/623b7361abfa

The failures happened on Linux, OSX, and Windows desktop builds.
(In reply to Wes Kocher (:KWierso) from comment #6)
> (In reply to Wes Kocher (:KWierso) from comment #5)
> > On several of the mochitest-bc jobs on this push, this failure showed up:
> > https://tbpl.mozilla.org/php/getParsedLog.php?id=32796690&tree=B2g-Inbound
> > 
> > So I backed out the patch from this bug in
> > https://hg.mozilla.org/integration/b2g-inbound/rev/623b7361abfa
> 
> The failures happened on Linux, OSX, and Windows desktop builds.

And on each platform that had the m-bc failure, Jetpack failed like this: https://tbpl.mozilla.org/php/getParsedLog.php?id=32797728&tree=B2g-Inbound
Hm, so it loks like the devtools actually need these files. Not sure how to make worthwhile progress for now, will revisit later.
Product: Add-on SDK → Firefox OS
triage: change component so blocking flag can be used.
1.3T? to keep in tarako triage radar
blocking-b2g: --- → 1.3T?
Whiteboard: [tarako][POVB]
triage: 1.3T+ as partner is requesting to lower storage size
blocking-b2g: 1.3T? → 1.3T+
Dave is there anyone on your team who might know how we can package only the files needed by DevTools code? 

There was a lot of work put into the loader to only package the modules actually used at runtime, so maybe something there we can leverage?
Flags: needinfo?(dtownsend+bugmail)
(In reply to Dietrich Ayala (:dietrich) from comment #11)
> Dave is there anyone on your team who might know how we can package only the
> files needed by DevTools code? 
> 
> There was a lot of work put into the loader to only package the modules
> actually used at runtime, so maybe something there we can leverage?

All we need is to know which modules devtools relies on. Someone could probably figure that out by adding some logging to the loader to say when it loads a module, run the devtools tests and see what it spits out. Then we just package those modules.
Flags: needinfo?(dtownsend+bugmail)
Assignee: nobody → fabrice
Component: General → Runtime
Fabrice, wonder if you can update the status of this bug? thanks
Flags: needinfo?(fabrice)
(In reply to Joe Cheng [:jcheng] from comment #13)
> Fabrice, wonder if you can update the status of this bug? thanks

Anyone that wants to help with doing what Dave says in comment 12 is welcome.
Flags: needinfo?(fabrice)
Thomas, is this something your team can also help with? Thanks
Flags: needinfo?(ttsai)
WIP has this patch on 01/17: https://github.com/Seinlin/tarako-patches.git
commit 4541d9d5a22f68f996f165a43646fdfad86ec8a2
Author: Kai-Zhen Li <kli@mozilla.com>
Date:   Fri Jan 17 18:44:30 2014 +0800
Flags: needinfo?(ttsai)
wonder if we can land this? thanks
Flags: needinfo?(fabrice)
(In reply to Joe Cheng [:jcheng] from comment #17)
> wonder if we can land this? thanks

No, the previous failure has not been fixed.
Flags: needinfo?(fabrice)
How many ROM size can this patch reduce?
ni kchen, is it something you can help looking into further? thanks
Flags: needinfo?(kchen)
Assignee: fabrice → kchen
Flags: needinfo?(kchen)
Hey Kan-Ru, here's the last try build I did Friday:
https://tbpl.mozilla.org/?tree=Try&rev=fd7114bbc0eb

I think that the patch actually has a bug, and removes files on platforms other than b2g, which should not happen.
Comment on attachment 8356744 [details] [diff] [review]
sdk-b2g.patch

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

I pushed another try, I think this time it will pass

https://tbpl.mozilla.org/?tree=Try&rev=3e31af3102bd

::: addon-sdk/copy_source.py
@@ +12,5 @@
>  
>  topsrcdir = sys.argv[1]
>  source_dir = sys.argv[2]
>  target_dir = sys.argv[3]
> +isB2G = sys.argv[4]

This is a string but should be a integer

@@ +35,5 @@
>      if not filenames:
>          continue
>      dirpath = dirpath.replace(os.sep, '/')
>      relative = dirpath[len(source_dir):]
> +    if isB2G and relative in [

This test will always be true so we skipped these files on all platforms
Fabrice asked whether setting debugging in adb+devtools still works properly with the sdk patch. I tested it and it works :)
Ying, are you ok to uplift this? Thanks

--
Keven
Flags: needinfo?(ying.xu)
https://hg.mozilla.org/mozilla-central/rev/060c0e523770
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.5 S1 (9may)
Flags: needinfo?(ying.xu)
You need to log in before you can comment on or make changes to this bug.