Fix find-dupes to exclude l10n

RESOLVED FIXED in Firefox 56

Status

RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: gandalf, Assigned: gandalf)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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

The log shows: https://pastebin.mozilla.org/9026097
(Assignee)

Comment 1

2 years ago
Created attachment 8882664 [details] [diff] [review]
WIP v1

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
(Assignee)

Comment 2

2 years ago
Chris, any ideas on what is missing?
Flags: needinfo?(catlee)
(Assignee)

Updated

2 years ago
Blocks: 1358824
(Assignee)

Updated

2 years ago
Attachment #8882664 - Attachment is obsolete: true
(Assignee)

Comment 3

2 years ago
got help from :catlee. The patch is coming.
Flags: needinfo?(catlee)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

2 years ago
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 6

2 years ago
mozreview-review
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+
(Assignee)

Comment 7

2 years ago
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+

Comment 9

a year ago
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7cb5f70b086d
Fix find-dupes to exclude l10n. r=catlee

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7cb5f70b086d
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
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 hidden (mozreview-request)
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 22

a year ago
mozreview-review
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+

Comment 23

a year ago
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7f3958d4703a
Fix find-dupes to exclude l10n. r=catlee

Comment 24

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7f3958d4703a
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
Resolution: --- → FIXED
(Assignee)

Updated

a year ago
Blocks: 1380937

Updated

9 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.