Closed Bug 1377543 Opened 3 years ago Closed 3 years ago

Fix find-dupes to exclude l10n

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

The find-dupes code breaks packaging when packaging multilingual Firefox Desktop (bug 1358824).

The log shows: https://pastebin.mozilla.org/9026097
Attached patch WIP v1 (obsolete) — Splinter Review
I was able to write a small patch that was supposed to fix things.
Unfortunately, when running with this change the packaging breaks with: https://pastebin.mozilla.org/9026120

My guess is that whatever this patch fixes makes the build system go further (since it doesn't stop on the first dupe and collects more) but eventually it errors out anyway for some reason.
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Chris, any ideas on what is missing?
Flags: needinfo?(catlee)
Attachment #8882664 - Attachment is obsolete: true
got help from :catlee. The patch is coming.
Flags: needinfo?(catlee)
Comment on attachment 8882721 [details]
Bug 1377543 - Fix find-dupes to exclude l10n.

Catlee agreed to be the reviewer here, but I'd like to make sure that Pike is also OK with this change.

Pike, can you confirm that it's ok for us to do that? This unblocks packaging multilocale builds.
Attachment #8882721 - Flags: feedback?(l10n)
Comment on attachment 8882721 [details]
Bug 1377543 - Fix find-dupes to exclude l10n.

https://reviewboard.mozilla.org/r/153812/#review158992

Are you going to remove the now-ignored files from the various allowed-dupes.mn files in-tree?
Attachment #8882721 - Flags: review?(catlee) → review+
Yeah, will remove in a follow-up patch!

Will wait for Pike's f? before landing.

Thank you for the review! :)
Comment on attachment 8882721 [details]
Bug 1377543 - Fix find-dupes to exclude l10n.

I had doubts on l10n being part of this, as en-* might just as well be the same for good reasons as en-US.

Fine with dropping find-dupes.py for l10n files in general.

This would also obsolete bug 1374773, fwiw.
Attachment #8882721 - Flags: feedback?(l10n) → feedback+
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7cb5f70b086d
Fix find-dupes to exclude l10n. r=catlee
https://hg.mozilla.org/mozilla-central/rev/7cb5f70b086d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Duplicate of this bug: 1374773
Backed out for breaking Android nightlies:

https://hg.mozilla.org/integration/mozilla-inbound/rev/876f6d3d160852f68d6da1ad7353d8934bf27e84 (planned to merge it around later today)

central with failing builds: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=06c1a0dc0fd8719ffa2d9aa2de75f32de5657102&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=112764715&repo=mozilla-central

[task 2017-07-08T10:54:10.173179Z] 10:54:10     INFO -  10:54:10     INFO -  WARNING: Found 117 duplicated files taking 1286975 bytes (841240 compressed)
[task 2017-07-08T10:54:10.173362Z] 10:54:10     INFO -  10:54:10     INFO -  ERROR: The following duplicated files are not allowed:
[task 2017-07-08T10:54:10.173543Z] 10:54:10     INFO -  10:54:10     INFO -  chrome/as/locale/as/browser/overrides/intl.css
[task 2017-07-08T10:54:10.173727Z] 10:54:10     INFO -  10:54:10     INFO -  chrome/cy/locale/cy/browser/overrides/intl.css
Flags: needinfo?(gandalf)
Silly me would have thought that since this code was originally put in to kludge android builds to work, a patch to remove it would have at least been tested on android builds before landing.
(In reply to Bill Gianopoulos [:WG9s] from comment #13)
> Silly me would have thought that since this code was originally put in to
> kludge android builds to work, a patch to remove it would have at least been
> tested on android builds before landing.

If you looked at the MRB you'd find the try run (including Android) that I tested it on before landing:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=79604c8ee1b2

If you had another message except of the passive aggressive note, then I didn't catch it.
Comment on attachment 8882721 [details]
Bug 1377543 - Fix find-dupes to exclude l10n.

:catlee - I changed the test for l10n to just look for `/locale/` path. It seems like it's more reliable versus different formats that end up in locale directory.

Sorry for the trouble :aryx and :catlee!
Flags: needinfo?(gandalf)
Attachment #8882721 - Flags: review+ → review?(catlee)
I wonder if it'd be better to take the UnpackFinder, and get the manifests via .find('**/chrome.manifest'), and then to iterate over the entries to get all mozpack.chrome.manifest.ManifestLocale and inspect those for path patterns to exclude.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #15)
> (In reply to Bill Gianopoulos [:WG9s] from comment #13)
> If you had another message except of the passive aggressive note, then I
> didn't catch it.

Sorry.
(In reply to Bill Gianopoulos [:WG9s] from comment #19)
> (In reply to Zibi Braniecki [:gandalf][:zibi] from comment #15)
> > (In reply to Bill Gianopoulos [:WG9s] from comment #13)
> > If you had another message except of the passive aggressive note, then I
> > didn't catch it.
> 
> Sorry.

I was venting my frustration at the lack of traction on bug 1374773 which, unfortunately seems to be the norm for Mozilla-central bugs that only impact comm-central builds.  I was encouraged that this landed and was in the course of producing a patch to remove the -w form find-dupes in SeaMonkey builds when this got backed out for issues in Android multi-locale nightlies.
(In reply to Axel Hecht [:Pike] from comment #18)
> I wonder if it'd be better to take the UnpackFinder, and get the manifests
> via .find('**/chrome.manifest'), and then to iterate over the entries to get
> all mozpack.chrome.manifest.ManifestLocale and inspect those for path
> patterns to exclude.

That would be more complex than just scanning for `/locale/` in the path.

I'm not sure if it's worth it as all locale code is in the directory and no locale code is outside of it. So it seems to me that those two solutions are equal, while the path checking is simpler.

:catlee - your call. I can write the code proposed by Axel if you prefer it.
Comment on attachment 8882721 [details]
Bug 1377543 - Fix find-dupes to exclude l10n.

https://reviewboard.mozilla.org/r/153812/#review161022
Attachment #8882721 - Flags: review?(catlee) → review+
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7f3958d4703a
Fix find-dupes to exclude l10n. r=catlee
https://hg.mozilla.org/mozilla-central/rev/7f3958d4703a
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.