Closed Bug 1032681 Opened 7 years ago Closed 3 years ago

copy selected file if package.json has gaia_shared


(Firefox OS Graveyard :: Gaia::Build, defect)

Not set


(Not tracked)



(Reporter: yurenju, Unassigned)




(1 file)

now we have new module webapp-shared to pick files from shared directory to build_stage, but we have a regression for copying selected files if gaia_shared.json exists since we add webapp-shared into pre-app.js but not post-app.js, we should move it to post-app.js and make gaia_shared.json works again.

as I know we have two apps using gaia_shared.json: clock and email app. we should ensure it works for those apps before we land it.
We need to make sure all shared resource are prepared before executing r.js (settings, camera, email ...) and also prevent default r.js logic that will remove merged files.

I can't find suitable require config for it, so the only way I think of is to add copy rule in each app's makefile.

Or we should wait for JSify all app makefile and let app decide whether to use webapp-shared or some other customized rule for copy things.
Assignee: nobody → gduan
Summary: copy selected file if gaia_shared.json exists in app directory → copy selected file if package.json has gaia_shared
Attached file PR to master
Hi Ricky,
could I have your review on this patch? Thanks.
Attachment #8556390 - Flags: review?(ricky060709)
Comment on attachment 8556390 [details] [review]
PR to master

I leaved an API nit on Github. Please fix it before landing!!
Attachment #8556390 - Flags: review?(ricky060709) → review+
This patch doesn't fix the original issue, do you have another patch in queue?

How do you think apps are going to use this new attribute in package.json?
Today, clock and email are generating shared file list dynamically.
If we keep doing that, it means that we would have to modify package.json during runtime, that doesn't sounds like a great option as package.json is on git and would appear modified as soon as we run the build system.
It seems clock and email don't really use our build/webapp-shared.js to copy shared data. So I think it won't impact on this patch.
I may not have all the correct context, but some background from email:

Email used to use the gaia_shared.json method until it no longer worked, in which it moved away from it. However, I would like to return to email generating a gaia_shared.json and using that for what shared resources were needed. This would eliminate having to manually track the resources as comments in email/index.html.

In that scenario, I think it is best if the file was gaia_shared.json instead of package.json, unless there are other design factors in play. Originally I believe the email app generated the gaia_shared.json in the build_stage/email directory and it was read from that location, so there were not concerns about modifying files that were tracked by source control.
Currently we handle shared related things before app's own build process, so I think handling gaia_shared.json from apps' folder might not affect anything. 

Also, I'm wondering why email would like to dynamically generate gaia_shared.json in build time? It's to prevent deleted by r.js or some other reason?
Flags: needinfo?(jrburke)
Correct, this issue was filed because when the shared copying was moved to the pre step, before the app's build process, it broke the gaia_shared.json support.

For an app using a module system, the JS dependencies are specified in JS files, not in the HTML, and for r.js usage, it can inline @import CSS in the CSS files. These are both used by email. The regular gaia shared copying only knew how to process things mentioned in the HTML file.

So the gaia_shared.json was a way for the app to generate what it used from the shared directory so that those items would make it into the app zip. Without it, the gaia build system would delete shared things it did not know about.

Since gaia_shared.json does not currently work, the email app had to manually place references to the shared items in comments in the index.html so that the shared items would show up.

So it would be nice to have gaia_shared.json support again to avoid that duplication (dependency reference in JS, comment in HTML). It usually causes developer confusion when they add it to the JS dependency but then does not work after a build, because they did not know or forgot they also need to mention it in the index.html.

If not gaia_shared.json support, then maybe some way for an app to indicate that it does not want the gaia build system to do the /shared pruning, and then the app's build file could decide to construct the shared folder used for the build result. 

I originally did not go down that route though since the shared directory work could have also involved some things like l10n optimizations. If that is no longer the case then perhaps an option for the app to opt-out of the shared directory work would be an alternative.
Flags: needinfo?(jrburke)
Thanks James, 
Offline discussed with Ricky, we think to enable gaia_shared.json and put comments in html would confuse developer. I would like to close gaia_shared support in this bug. 

As u mentioned, I think app should be able to enable/disable each build task by some config file. like r.js, it's able to merge files, optimize file and do file transferring, There's actually no need to do duplicate things in main building thread. We'll put it into our backlog.
I may have misunderstood comment 9, but the gaia_shared.json support would avoid the need for the comments in the HTML. However, if you prefer to go about the general problem via flags to turn off parts of the build, that is fine too.
In order to make gaia_shared.json works again, we probably need to implement ...

1. let post-app be able to manage build_app step
2. webapp-shared can be put into post-app
3. let app decide what steps in post-app need to run and how they order.
Deassign myself.
comment 11 should be implemented in NewGaiaBuild, but the schedule would probably be postponed due to resource reallocation.
Assignee: gduan → nobody
Firefox OS is not being worked on
Closed: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.