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

Status: RESOLVED FIXED

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

Description

[[:fabrice] Fabrice Desré]

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.

Comment 1

[[:fabrice] Fabrice Desré]

Attachment: sdk-b2g.patch

That works here for b2g, I'll push to try to make sure I'm no breaking the world.

Comment 2

[Dave Townsend [:mossop]]

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.

Comment 3

[Gregory Szorc [:gps]]

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.

Comment 4

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/9da8bc670c03

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(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.

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(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

Comment 8

[[:fabrice] Fabrice Desré]

Hm, so it loks like the devtools actually need these files. Not sure how to make worthwhile progress for now, will revisit later.

Comment 9

[Joe Cheng [:jcheng] (please needinfo)]

triage: change component so blocking flag can be used.
1.3T? to keep in tarako triage radar

Comment 10

[Joe Cheng [:jcheng] (please needinfo)]

triage: 1.3T+ as partner is requesting to lower storage size

Comment 11

[Dietrich Ayala (:dietrich)]

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?

Comment 12

[Dave Townsend [:mossop]]

(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.

Comment 13

[Joe Cheng [:jcheng] (please needinfo)]

Fabrice, wonder if you can update the status of this bug? thanks

Comment 14

[[:fabrice] Fabrice Desré]

(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.

Comment 15

[Joe Cheng [:jcheng] (please needinfo)]

Thomas, is this something your team can also help with? Thanks

Comment 16

[thomas tsai]

WIP has this patch on 01/17: https://github.com/Seinlin/tarako-patches.git
commit 4541d9d5a22f68f996f165a43646fdfad86ec8a2
Author: Kai-Zhen Li <[e-mail]>
Date:   Fri Jan 17 18:44:30 2014 +0800

Comment 17

[Joe Cheng [:jcheng] (please needinfo)]

wonder if we can land this? thanks

Comment 18

[[:fabrice] Fabrice Desré]

(In reply to Joe Cheng [:jcheng] from comment #17)
> wonder if we can land this? thanks

No, the previous failure has not been fixed.

Comment 19

[James Zhang (Spreadtrum)]

How many ROM size can this patch reduce?

Comment 20

[Joe Cheng [:jcheng] (please needinfo)]

ni kchen, is it something you can help looking into further? thanks

Comment 21

[[:fabrice] Fabrice Desré]

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 22

[Kan-Ru Chen [:kanru] (UTC+9)]

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

Comment 23

[Kan-Ru Chen [:kanru] (UTC+9)]

https://hg.mozilla.org/integration/b2g-inbound/rev/060c0e523770

Comment 24

[Kan-Ru Chen [:kanru] (UTC+9)]

Fabrice asked whether setting debugging in adb+devtools still works properly with the sdk patch. I tested it and it works :)

Comment 25

[Keven Kuo [:kkuo]]

Ying, are you ok to uplift this? Thanks

--
Keven

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/060c0e523770

Comment 27

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/96ef8c832ff5