Closed Bug 1116613 Opened 9 years ago Closed 7 years ago

Develop a smart strategy for building language timestamps at build time

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::L10n
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: zbraniecki, Unassigned)

References

(
URL
)

Details

Attachments

(1 file, 2 obsolete files)

[gaia] zbraniecki:1116613-smart-strategy-for-l10n-timestamps > mozilla-b2g:master
46 bytes, text/x-github-pull-request
rickychien
: review+
Details | Review 
Spin off of bug 1115807.

We want to develop a better strategy for building timestamps that are marking languages at build time.

The initial strategy is to use current datetime. That makes the builds not reproducible.

A better approach is to use:

1) For builds with locales from gaia source repository (no LOCALE_BASEDIR) use gaia's git/hg commit datetime 
2) For builds using LOCALE_BASEDIR use git/hg commit datetime for each locale separately
3) As a fallback (if the directory is not a git/hg repo), use current datetime.
Assignee: nobody → gandalf
Depends on: 1115807
One unknown here is all the systems on which we build Gaia. I have no idea how our build system works with Windows shell, or anything beyond Linux/Mac for the matter.

I guess in order to get hg/git we can either try to find some JS binding, or use command line and execute the command. Both sounds scary tbh, but I'll tackle it if I get confirmation on the proposed strategy from you guys.
Flags: needinfo?(stas)
Flags: needinfo?(poirot.alex)
Flags: needinfo?(l10n)
(In reply to Zibi Braniecki [:gandalf] from comment #0)
proach is to use:
> 
> 1) For builds with locales from gaia source repository (no LOCALE_BASEDIR)
> use gaia's git/hg commit datetime 
> 2) For builds using LOCALE_BASEDIR use git/hg commit datetime for each
> locale separately
> 3) As a fallback (if the directory is not a git/hg repo), use current
> datetime.

I wonder if we can get away with just using the timestamp of the Gaia commit used for building in all cases.  My thinking is that the worst case scenario in this case is that the user then installs a langpack which has the same files as already present on the device.

             A                    C
  Gaia: -----+--------------------+---->
             |                    |
              the Gaia commit used to build Gaia (date A)
                                  |
                                   a newer commit (date C)

                           B
  L10n: -------------------+----------->
                           |
                            the l10n changeset used to build Gaia (date B)


This isn't ideal, but has the advantage of being quite simple to implement.  The upside is that even if there's a new commit in Gaia from which the build is created (C), our build docs instruct to take the tip of the l10n repos so we'll end up with the most up-to-date localizations which can be then updated too.
Flags: needinfo?(stas)
You couldn't update a localization on a stale gaia branch anymore, though. I'm still with "timestamp of checked out revision or utcnow()"
Flags: needinfo?(l10n)
(In reply to Zibi Braniecki [:gandalf] from comment #1)
> One unknown here is all the systems on which we build Gaia. I have no idea
> how our build system works with Windows shell, or anything beyond Linux/Mac
> for the matter.

We run everywhere: linux, mac and windows.

> 
> I guess in order to get hg/git we can either try to find some JS binding, or
> use command line and execute the command. Both sounds scary tbh, but I'll
> tackle it if I get confirmation on the proposed strategy from you guys.

We should do what we already do for settings (and other such usages): call command line programs.
Like apps/privacy-panel/build/build.js and apps/settings/build/build.js.
It appears to be working fine so far.
Flags: needinfo?(poirot.alex)
I have the solution mostly working on a branch now, but because it triggers us to be async, I'm leaning toward not doing this in 2.2 unless you guys think it's necessary.

It requires turning most of multilocale into async code to facilitate async Commander operations.

Stas, Alexandre, do you think we can keep the timestamp from buildtime for 2.2 and land this only on master?
URL: https://github.com/zbraniecki/gaia/tr...
Flags: needinfo?(stas)
Flags: needinfo?(poirot.alex)
Zibi:  my fear is that if we leave this as-is, we'll make it hard or outright impossible to use langpacks on nightly builds, because the versions of locales bundled in Gaia will almost always be newer than the langpacks'.

Ricky, what are your thoughts on running async commands via utils.Commander in multilocale.js?  Could we make multilocale.js's "execute" function return a promise and then change the flow here:

https://github.com/mozilla-b2g/gaia/blob/master/build/post-app.js#L12

?
Flags: needinfo?(stas) → needinfo?(ricky060709)
(In reply to Staś Małolepszy :stas from comment #6)
> Ricky, what are your thoughts on running async commands via utils.Commander
> in multilocale.js?  Could we make multilocale.js's "execute" function return
> a promise and then change the flow here:
> 
> https://github.com/mozilla-b2g/gaia/blob/master/build/post-app.js#L12
> 
> ?

Yes, or we can use:
  utils.processEvents(function () {
    return { wait: false };
  });
trick to synchronously from within multilocale.js, if that helps.
I don't see the switch form sync to async to be a risky move, it will be more about the code your are introducing, calling commands from JS... that may fail on developers or CI environment.
Flags: needinfo?(poirot.alex)
You can land it on master, see how it goes and than decide if that's worth uplifting.
Comment on attachment 8569443 [details] [review]
[gaia] zbraniecki:1116613-smart-strategy-for-l10n-timestamps > mozilla-b2g:master

Ok, here's the patch. It seems to work for me quite well and does take the HG/GIT rev date if possible.
Attachment #8569443 - Flags: review?(stas)
Attachment #8569443 - Flags: review?(poirot.alex)
One side effect of this change is that now webapp-optimize L10n errors are not exiting the app (because they are executed after return), they are just reported in the console.
Comment on attachment 8569443 [details] [review]
[gaia] zbraniecki:1116613-smart-strategy-for-l10n-timestamps > mozilla-b2g:master

I left a few comments in the pull request.  Overall this looks good but can you please try to use processEvents as suggested by :ochameau in comment 7?

It looks like Commander uses processEvents as well so maybe using Promises is not needed at all?

Also, can processEvents be used to work around errors not being fatal?

https://github.com/mozilla-b2g/gaia/blob/master/build/utils-xpc.js#L685-L706
Attachment #8569443 - Flags: review?(stas) → review+
(In reply to Staś Małolepszy :stas from comment #12)
> Comment on attachment 8569443 [details] [review]
> [gaia] zbraniecki:1116613-smart-strategy-for-l10n-timestamps >
> mozilla-b2g:master
> 
> I left a few comments in the pull request.  Overall this looks good but can
> you please try to use processEvents as suggested by :ochameau in comment 7?

What would be the advantage of that?

> It looks like Commander uses processEvents as well so maybe using Promises
> is not needed at all?

We need to preserve order of operations, not just wait for the outcome.

> Also, can processEvents be used to work around errors not being fatal?

I'll give it a try, but I'm not sure. If we are async, it means that we have a code spawned from the main thread running after the main thread returned, not exited.
(In reply to Zibi Braniecki [:gandalf] from comment #13)
> > I left a few comments in the pull request.  Overall this looks good but can
> > you please try to use processEvents as suggested by :ochameau in comment 7?
> 
> What would be the advantage of that?

I'll defer to :ochameau and :rickychien, since they have a much better understanding of how the build system works and what the preferred style of handing async operations is.  It just seems that utils.processEvents is used a lot in many places whereas then-ables are not.
(In reply to Zibi Braniecki [:gandalf] from comment #13)
> (In reply to Staś Małolepszy :stas from comment #12)
> > Comment on attachment 8569443 [details] [review]
> > [gaia] zbraniecki:1116613-smart-strategy-for-l10n-timestamps >
> > mozilla-b2g:master
> > 
> > I left a few comments in the pull request.  Overall this looks good but can
> > you please try to use processEvents as suggested by :ochameau in comment 7?
> 
> What would be the advantage of that?

I agree that async is a good thing and I'd like to see build system to adopt async API in the future.
For now, we're focusing on how to make our code style more consistent, so please use utils.processEvents instead. =)

> 
> > It looks like Commander uses processEvents as well so maybe using Promises
> > is not needed at all?

You're right!

> 
> We need to preserve order of operations, not just wait for the outcome.
> 
> > Also, can processEvents be used to work around errors not being fatal?
> 
> I'll give it a try, but I'm not sure. If we are async, it means that we have
> a code spawned from the main thread running after the main thread returned,
> not exited.
Flags: needinfo?(ricky060709)
Comment on attachment 8569443 [details] [review]
[gaia] zbraniecki:1116613-smart-strategy-for-l10n-timestamps > mozilla-b2g:master

Removing my r+ for now because I noticed that you use the same timestamp for all locales instead of getting one for each independently.  Is this on purpose?
Attachment #8569443 - Flags: review+
Comment on attachment 8569443 [details] [review]
[gaia] zbraniecki:1116613-smart-strategy-for-l10n-timestamps > mozilla-b2g:master

Looks good overall (There is no more l10n noise in diff between two changeset!!) but the promise refactoring looks useless as Commander should be synchronous with subprocess used like this... Also promise.all is weak, see my comment in PR. processEvent looks like dark magic, but is less prone to errors.

I'll delegate the final r+ to ricky who is refactoring.
There is a promise module in node, but I have no idea if that's compatible with ES6 one, nor if you plan to start using it? It is the kind of viral pattern that you have to use everywhere when you start with it...
Attachment #8569443 - Flags: review?(poirot.alex) → review?(ricky060709)
Ok, thanks a lot for your feedback guys!

I updated the patch, which, apparently is synchronous anyway :) The new patch is smaller and simpler thanks to this.

Ricky, will you have time to review it soon? We need to land it on master to request 2.2 uplift and that's past due :(
Flags: needinfo?(ricky060709)
Comment on attachment 8569443 [details] [review]
[gaia] zbraniecki:1116613-smart-strategy-for-l10n-timestamps > mozilla-b2g:master

Sorry for my late reply. The patch seems good to me. Let's land it! 

BTW, for my concern is after applying this patch, "System JS : Error ..." outputs too many times more than before although it seems not to hurt anything. =(
Flags: needinfo?(ricky060709)
Attachment #8569443 - Flags: review?(ricky060709) → review+
Comment on attachment 8569443 [details] [review]
[gaia] zbraniecki:1116613-smart-strategy-for-l10n-timestamps > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 1115796 (langpacks)
[User impact] if declined: it may be potentially harder for partners to release langpacks depending on their release infrastructure. This patch clears the "langpack is newer than bundled locale"
[Testing completed]: locally, gaia-try
[Risk to taking this patch] (and alternatives if risky): none known
[String changes made]: none
Attachment #8569443 - Flags: approval-gaia-v2.2?(bbajaj)
had to revert this for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=1453161&repo=b2g-inbound
Status: RESOLVED → REOPENED
Flags: needinfo?(gandalf)
Resolution: FIXED → ---
Ok, that's beyond my understanding of our buildsystem. Try was green, builds work on linux and mac locally.

I need build system masters help with that. Ricky, Alexandre, do you have any clues on what causes it?
Flags: needinfo?(ricky060709)
Flags: needinfo?(poirot.alex)
Flags: needinfo?(gandalf)
Oops! That's my first time seeing this.

https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=3adc182bb7fd

As far as I know, it seems the buildbot environments are different, and only one "B2G Desktop Linux x64 debug" passed. So it might be a worst case that you should request a loaner machine [1] to look into the buildbot's issue and find out what's happened.

[1] https://wiki.mozilla.org/ReleaseEngineering/How_To/Request_a_slave
Flags: needinfo?(ricky060709)
Comment on attachment 8569443 [details] [review]
[gaia] zbraniecki:1116613-smart-strategy-for-l10n-timestamps > mozilla-b2g:master

Ok, I'm taking off 2.2 approval request. I would like to have it in 2.2 because it potentially may work better for partners, but it's not a blocker.

Let's try to figure it out for master anyway. It seems to me that we're hitting the limit of subprocesses spawned, which makes sense since we spawn the number of locales times the number of apps processes.

It would be better to cache the datetime for each locale so that we don't have to get it via subprocess for each app.

Is there a way to cache variables between apps at buildtime?
Flags: needinfo?(ricky060709)
Attachment #8569443 - Flags: approval-gaia-v2.2?(bbajaj)
If it hits the limit of subprocesses spawned, you should try to move "var hg = new utils.Commander('hg');" out of getLastHGCommitTimestamp() instead. Theoretically, that should have a little bit performance improvement too.
Flags: needinfo?(ricky060709)
Attachment #8569443 - Attachment is obsolete: true
Tomcat, I updated the patch to Ricky's suggestion. It's still of course green on gaia-try. Can I land it again to see if it'll blow b2g-inbound?
Flags: needinfo?(cbook)
(In reply to Zibi Braniecki [:gandalf] from comment #28)
> Tomcat, I updated the patch to Ricky's suggestion. It's still of course
> green on gaia-try. Can I land it again to see if it'll blow b2g-inbound?

yeah sure thats fine for me
Flags: needinfo?(cbook)
Thanks Ryan!

Ricky, seems that I need to further reduce the number of subprocesses.

I think that the only clear solution would be to collect the timestamps between apps. I don't know if there's any way to do that in-memory since we're parallelizing app building.

Alternatively, I could create some temporary text file and collect timestamps for paths there and then use the cached values from that file instead of collecting them for each app separately.

Would that work for you? If so, where should I put this file? And at which step I should do this? Ideally, I'd do this in a step that is launched only once per build, not once per app. (like multilocale.js).
Flags: needinfo?(ricky060709)
I saw some potential issue in here.

1. As a contributor, try to download zip/gz/bz2 from http://hg.mozilla.org/gaia-l10n wouldn't contain any .hg database. Thus, we cannot get timestamp by hg command.
2. As above, not everyone especially contributors have pre-installed hg in their machine.

Thus, I think using hg is not good enough to solve this issue.
Flags: needinfo?(ricky060709)
Flags: needinfo?(poirot.alex)
Ricky, that's why we have a fallback mechanism. In the worst case, we will do what we do right now - use the build time timestamp.

But if we do have a repo and hg/git we can be more predictable, generate reproducible builds (which is what Alexandre wanted) and make it easier for release engineers to get builds that will have proper timestamps vs. langpack timestamps.

Stas, what do you think?
Flags: needinfo?(stas)
I agree, we should prefer hg clones over archive downloads.  Can you document it under https://developer.mozilla.org/en-US/Firefox_OS/Building#Building_multilocale ?  Something to the effect of "Cloning l10n repos is required for the multilocale build to work correctly.  Some features (e.g. langpacks) make use of the metadata stored in the Mercurial repositories."

Also, would it make sense to console.warn about subdirectories if LOCALE_BASEDIR not being hg clones?
Flags: needinfo?(stas)
Ricky, it seems to me that I and stas agree here on that we'd like to use hg/git revision timestamp whenever possible. Would you agree with Stas's and mine reasoning?

If so, can you help me with my questions from comment 33?
Flags: needinfo?(ricky060709)
How about getting timestamp in build_staged/timestamp.json which used by our incremental build mechanism (build/rebuild.js)
Flags: needinfo?(ricky060709)
Attachment #8574933 - Attachment is obsolete: true
Comment on attachment 8584126 [details] [review]
[gaia] zbraniecki:1116613-smart-strategy-for-l10n-timestamps > mozilla-b2g:master

I updated the patch to use the build_stage/timestamps.json.

There's enough code to manage timestamps that I encapsulated it in its own class, mostly for readability and maintainability reasons.

It reads timestamps, edits timestamps and write timestamps.

Ricky, can you review if that looks good? I'd like to give it one last try to get it in for 2.2 so that we have repeatable builds.
Attachment #8584126 - Flags: review?(ricky060709)
So, I had to switch to use our custom timestamp-l10n.json file to prevent some read/write errors with JSON.
It seems that it works well.

Ricky, can you review that? If this will work, I'd like to uplift it to 2.2
Flags: needinfo?(ricky060709)
Comment on attachment 8584126 [details] [review]
[gaia] zbraniecki:1116613-smart-strategy-for-l10n-timestamps > mozilla-b2g:master

It looks okay to me. Just a little thing confuse me so I've leaved comment on Github.
Flags: needinfo?(ricky060709)
Attachment #8584126 - Flags: review?(ricky060709) → review+
I'm going to land it in order to test if gaia-inbound will work and prepare for 2.2 uplift.

We'll keep the conversation with Ricky and I may file a follow up depending on the outcome.
Keywords: checkin-needed
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
We got another error on b2g-inbound, this time something related to our subprocess.

I pushed a follow up: https://github.com/mozilla-b2g/gaia/commit/1eb5cbd3353a17be425e65fcb18eb2df2ce315d8

Hopefully this will make the error go away.
backed out https://github.com/lightsofapollo/gaia/commit/91b0d98d72209b4ffa5d42b8694b4df107d27b60

--

I am talking to stas on IRC but the short version is we should probably be testing the same set of locales that we run builds on so we can catch this prior to BI.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to James Lal [:lightsofapollo] from comment #46)
> I am talking to stas on IRC but the short version is we should probably be
> testing the same set of locales that we run builds on so we can catch this
> prior to BI.

Can you provide link to a failure log? It's hard to debug without it :/
Flags: needinfo?(jlal)
Assignee: gandalf → nobody
Flags: needinfo?(jlal)
Status: REOPENED → RESOLVED
Closed: 9 years ago → 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: