Closed Bug 1881094 Opened 5 months ago Closed 26 days ago

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

Categories

(Fenix :: General, task, P3)

All
Android
task

Tracking

(firefox129 fixed)

RESOLVED FIXED
129 Branch
Tracking Status
firefox129 --- fixed

People

(Reporter: jcristau, Assigned: gbrown)

References

(Blocks 1 open bug)

Details

Attachments

(8 files)

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.

Assignee: nobody → gbrown
Keywords: leave-open
Pushed by gbrown@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6779954b3f81
2. remove whitespace exclusions for firefox-android r=android-reviewers,gl
Pushed by gbrown@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c230ec8f9514
3. accept rejected-words exclusions for firefox-android r=Standard8

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?

Flags: needinfo?(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.

Flags: needinfo?(standard8)
Pushed by gbrown@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/165dba755ef0
1. remove codespell exclusions for firefox-android r=android-reviewers,geckoview-reviewers,gl,m_kato

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

Priority: -- → P3
See Also: → 1893763
Pushed by gbrown@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/91b0084cd5fb
5. remove stylelint exclusions for firefox-android r=android-reviewers,frontend-codestyle-reviewers,gl
Attachment #9395558 - Attachment description: Bug 1881094 - 4. remove prettier exclusions for firefox-android → Bug 1881094 - 4. remove and revise prettier exclusions for firefox-android
Pushed by gbrown@mozilla.com:
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

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
See Also: → 1895746

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

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

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.

Pushed by gbrown@mozilla.com:
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

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?

Flags: needinfo?(standard8)
Flags: needinfo?(gl)
Flags: needinfo?(gijskruitbosch+bugs)

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.

(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 */
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(standard8)
Flags: needinfo?(gl)
Keywords: leave-open
Pushed by gbrown@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 26 days ago
Resolution: --- → FIXED
Target Milestone: --- → 129 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: