1881094 - re-enable linting for excluded firefox-android files after migration to m-c

Status: RESOLVED FIXED

(Fenix :: General, task, P3)

Description

[Julien Cristau [:jcristau]]

In bug 1825116, for expediency we're excluding some firefox-android files from the normal mach lint tasks.

After the merge we need to go back and figure out what needs to be fixed or excluded via things like Generated.txt or ThirdPartyPaths.txt.

Comment 1

[Geoff Brown [:gbrown]]

Attachment: Bug 1881094 - 1. remove codespell exclusions for firefox-android

Comment 2

[Geoff Brown [:gbrown]]

Attachment: Bug 1881094 - 2. remove whitespace exclusions for firefox-android

Comment 3

[Geoff Brown [:gbrown]]

Attachment: Bug 1881094 - 3. accept rejected-words exclusions for firefox-android

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6779954b3f81
2. remove whitespace exclusions for firefox-android r=android-reviewers,gl

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6779954b3f81

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c230ec8f9514
3. accept rejected-words exclusions for firefox-android r=Standard8

Comment 7

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/c230ec8f9514

Comment 8

[Geoff Brown [:gbrown]]

Attachment: Bug 1881094 - 4. remove and revise prettier exclusions for firefox-android

Comment 9

[Geoff Brown [:gbrown]]

We have many entries in license.yml to avoid missing-license lint errors on android resource files:
https://searchfox.org/mozilla-central/rev/1f27a4022f9f1269d897526c1c892a57743e650c/tools/lint/license.yml#48

Files like https://searchfox.org/mozilla-central/source/mobile/android/android-components/components/browser/engine-system/src/main/res/values-an/strings.xml are maintained in the android-l10n repo and imported into mozilla-central by automation. As such, I think it is appropriate for those paths to actually be in Generated.txt. However, if those paths are added to Generated.txt and removed from license.yml, the license linter tries to lint those paths (and reports many failures). Is that expected? Should paths be added to Generated.txt and duplicated in license.yml?

Comment 10

[Geoff Brown [:gbrown]]

Attachment: Bug 1881094 - 5. remove stylelint exclusions for firefox-android

Comment 11

[Mark Banner (:standard8)]

(In reply to Geoff Brown [:gbrown] (pto Apr 11-18) from comment #9)

We have many entries in license.yml to avoid missing-license lint errors on android resource files:
https://searchfox.org/mozilla-central/rev/1f27a4022f9f1269d897526c1c892a57743e650c/tools/lint/license.yml#48

Files like https://searchfox.org/mozilla-central/source/mobile/android/android-components/components/browser/engine-system/src/main/res/values-an/strings.xml are maintained in the android-l10n repo and imported into mozilla-central by automation. As such, I think it is appropriate for those paths to actually be in Generated.txt. However, if those paths are added to Generated.txt and removed from license.yml, the license linter tries to lint those paths (and reports many failures). Is that expected? Should paths be added to Generated.txt and duplicated in license.yml?

It looks like there's something wrong with either the license generator or how Generated.txt is working.

If I don't change license.yml but add mobile/android/android-components/components/**/src/main/res/**/ to the list in Generated.txt, then I start getting lots of errors. It seems to then ignore the mobile/android/**/build entry.

I think we should file fixing that as a separate bug under Developer Infrastructure: Lint and Formatting. We'll probably need a linter fix, so let's handle the license part there.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/165dba755ef0
1. remove codespell exclusions for firefox-android r=android-reviewers,geckoview-reviewers,gl,m_kato

Comment 13

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/165dba755ef0

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6779954b3f81

Comment 15

[Joe M [:jmahon]]

Note: marking this as P3 because it might take a while, but as of now, Geoff expects to be working on it continuously.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/91b0084cd5fb
5. remove stylelint exclusions for firefox-android r=android-reviewers,frontend-codestyle-reviewers,gl

Comment 17

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/91b0084cd5fb

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/727601e8500a
4. remove and revise prettier exclusions for firefox-android r=android-reviewers,frontend-codestyle-reviewers,gl,webcompat-reviewers,twisniewski

Comment 19

[tszentpeteri]

https://hg.mozilla.org/mozilla-central/rev/727601e8500a

Comment 20

[Geoff Brown [:gbrown]]

Attachment: Bug 1881094 - 7. cleanup firefox-android exclusions in license.yml

Cleanup license.yml by:

• adding license headers to gradle.properties: some gradle.properties files already have
  license headers; I have just copied those headers to files where they were missing
• adding license headers for a few .xml files
• moving the .../src/main/res/ exclusions to Generated.txt; these exclusions are primarily
  needed for the strings.xml files, which are updated from the android-l10n repo, so they
  seem more appropriate for Generated.txt

Comment 21

[Geoff Brown [:gbrown]]

(In reply to Mark Banner (:standard8) from comment #11)

I think we should file fixing that as a separate bug under Developer Infrastructure: Lint and Formatting. We'll probably need a linter fix, so let's handle the license part there.

Filed bug 1895746 for this.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/50569680f8e1
7. cleanup firefox-android exclusions in license.yml r=gl,android-reviewers

Comment 23

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/50569680f8e1

Comment 24

[Geoff Brown [:gbrown]]

Attachment: Bug 1881094 - 8. remove some eslint exclusions for firefox-android, using mach lint --fix

This eliminates some eslint errors on firefox-android code by running 'mach lint --linter=eslint --fix'.
The eslint --fix only addresses a few simple issues, but this allows us to remove some lint exclusions.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b2c1906d3f6e
8. remove some eslint exclusions for firefox-android, using mach lint --fix r=android-reviewers,frontend-codestyle-reviewers,webcompat-reviewers,denschub,Gijs,gl

Comment 26

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/b2c1906d3f6e

Comment 27

[Geoff Brown [:gbrown]]

Thanks for all your reviews and help with this bug!

Now "only" a small section of eslint exclusions remain: https://searchfox.org/mozilla-central/rev/f5251f16058e6307d92da02eed07ee91b9ee6243/.eslintignore#197-211

If those exclusions are removed, these eslint errors are produced: https://treeherder.mozilla.org/logviewer?job_id=459421658&repo=try&lineNumber=545

I'm not familiar with this code, nor do I have much experience with javascript in general. Any advice or suggestions for how to resolve these?

Comment 28

[Geoff Brown [:gbrown]]

wip with a quick fix for no-console and no-unused-vars:

https://treeherder.mozilla.org/jobs?repo=try&revision=c3051bf93c64265962eeb2cebf8664db6bbc034e

Most of the remaining issues are no-undef and no-unsanitized/property.

Comment 29

[:Gijs (he/him)]

(In reply to Geoff Brown [:gbrown] (pto, back Jun 10) from comment #27)

If those exclusions are removed, these eslint errors are produced: https://treeherder.mozilla.org/logviewer?job_id=459421658&repo=try&lineNumber=545

[task 2024-05-23T22:02:46.820Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/mobile/android/android-components/components/browser/icons/src/main/assets/extensions/browser-icons/icons.js:28:38 | 'currentIndex' is defined but never used. Allowed unused args must match /^_/u. (no-unused-vars)

Remove second + third arg (x3)

[task 2024-05-23T22:02:46.820Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/mobile/android/android-components/components/feature/accounts/src/main/assets/extensions/fxawebchannel/background.js:9:12 | 'browser' is not defined. (no-undef)

Add /* eslint-env webextensions */ at the top of the file. (same for fxawebchannel.js, readerview-background.js, readerview-content.js, readerview.js)

[task 2024-05-23T22:02:46.820Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/mobile/android/android-components/components/feature/readerview/src/main/assets/extensions/readerview/readerview.js:44:22 | 'Readability' is not defined. (no-undef)

Also add:

/* import-globals-from readability/readability-0.4.2.js */
/* import-globals-from readability/JSDOMParser-0.4.2.js */

Comment 30

[Geoff Brown [:gbrown]]

Attachment: Bug 1881094 - 9. remove most remaining firefox-android eslint exclusions

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0a6efe2cf258
9. remove most remaining firefox-android eslint exclusions r=android-reviewers,frontend-codestyle-reviewers,webcompat-reviewers,gl,Gijs,Standard8,twisniewski

Comment 32

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/0a6efe2cf258