1116613 - Develop a smart strategy for building language timestamps at build time

Status: RESOLVED WONTFIX

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

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

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.

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

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.

Comment 2

[Staś Małolepszy :stas]

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

Comment 3

[Axel Hecht [:Pike]]

You couldn't update a localization on a stale gaia branch anymore, though. I'm still with "timestamp of checked out revision or utcnow()"

Comment 4

[Alexandre Poirot [:ochameau]]

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

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

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?

Comment 6

[Staś Małolepszy :stas]

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

?

Comment 7

[Alexandre Poirot [:ochameau]]

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

Comment 8

[Alexandre Poirot [:ochameau]]

You can land it on master, see how it goes and than decide if that's worth uplifting.

Comment 9

[GitHub Autolander Bot]

Attachment: [gaia] zbraniecki:1116613-smart-strategy-for-l10n-timestamps > mozilla-b2g:master

Comment 10

[Zibi Braniecki [:zbraniecki][:gandalf]]

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.

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

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 12

[Staś Małolepszy :stas]

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

Comment 13

[Zibi Braniecki [:zbraniecki][:gandalf]]

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

Comment 14

[Staś Małolepszy :stas]

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

Comment 15

[Ricky Chien [:rickychien] (inactive)]

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

Comment 16

[Staś Małolepszy :stas]

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?

Comment 17

[Alexandre Poirot [:ochameau]]

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

Comment 18

[Zibi Braniecki [:zbraniecki][:gandalf]]

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

Comment 19

[Ricky Chien [:rickychien] (inactive)]

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

Comment 20

[Zibi Braniecki [:zbraniecki][:gandalf]]

Commit: https://github.com/mozilla-b2g/gaia/commit/3a21c4e1fb54ae2724893b57a5262af7a5c2557b
Merge: https://github.com/mozilla-b2g/gaia/commit/35177e699afab05db314b10dca69c3dd76646c00

Comment 21

[Zibi Braniecki [:zbraniecki][:gandalf]]

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

Comment 22

[Carsten Book [:Tomcat]]

had to revert this for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=1453161&repo=b2g-inbound

Comment 23

[Zibi Braniecki [:zbraniecki][:gandalf]]

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?

Comment 24

[Ricky Chien [:rickychien] (inactive)]

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

Comment 25

[Zibi Braniecki [:zbraniecki][:gandalf]]

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?

Comment 26

[Ricky Chien [:rickychien] (inactive)]

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.

Comment 27

[GitHub Autolander Bot]

Attachment: [gaia] zbraniecki:1116613-smart-strategy-for-l10n-timestamps > mozilla-b2g:master

Comment 28

[Zibi Braniecki [:zbraniecki][:gandalf]]

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?

Comment 29

[Carsten Book [:Tomcat]]

(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

Comment 30

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ok, landed again as:

Commit: https://github.com/mozilla-b2g/gaia/commit/b6d817f131d338e18b51925367ff42c8a7f417f6
Merge: https://github.com/mozilla-b2g/gaia/commit/7d5337556f92647cb12c99bc1e33a6cf959f4316

I'll observe b2g-inbound today and revert if needed.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Reverted for bustage.
Master: https://github.com/mozilla-b2g/gaia/commit/cef185cdc784c98255c0c68c89c7e1ca8a83a526

https://treeherder.mozilla.org/logviewer.html#?job_id=1496523&repo=b2g-inbound

Comment 32

[Ryan VanderMeulen [:RyanVM]]

A possibly more-clear build failure:
https://treeherder.mozilla.org/logviewer.html#?job_id=1496519&repo=b2g-inbound

Comment 33

[Zibi Braniecki [:zbraniecki][:gandalf]]

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

Comment 34

[Ricky Chien [:rickychien] (inactive)]

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.

Comment 35

[Zibi Braniecki [:zbraniecki][:gandalf]]

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?

Comment 36

[Staś Małolepszy :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?

Comment 37

[Zibi Braniecki [:zbraniecki][:gandalf]]

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?

Comment 38

[Ricky Chien [:rickychien] (inactive)]

How about getting timestamp in build_staged/timestamp.json which used by our incremental build mechanism (build/rebuild.js)

Comment 39

[GitHub Autolander Bot]

Attachment: [gaia] zbraniecki:1116613-smart-strategy-for-l10n-timestamps > mozilla-b2g:master

Comment 40

[Zibi Braniecki [:zbraniecki][:gandalf]]

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.

Comment 41

[Zibi Braniecki [:zbraniecki][:gandalf]]

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

Comment 42

[Ricky Chien [:rickychien] (inactive)]

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.

Comment 43

[Zibi Braniecki [:zbraniecki][:gandalf]]

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.

Comment 44

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/b79f351dcc98aff862d33c7ec32685555228dcf4

Comment 45

[Zibi Braniecki [:zbraniecki][:gandalf]]

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.

Comment 46

[James Lal [:lightsofapollo]]

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.

Comment 47

[James Lal [:lightsofapollo]]

https://github.com/lightsofapollo/gaia/commit/b27e9efb9df474b03642f4d59ad58438035ca141

Comment 48

[Zibi Braniecki [:zbraniecki][:gandalf]]

(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 :/