Closed Bug 1666491 Opened 4 years ago Closed 2 years ago

Create an eslint-build tester (tier 3) that depends on xpt artifacts - for checking the use of idl constants to ensure they are valid

Categories

(Firefox Build System :: Task Configuration, task)

task

Tracking

(firefox109 fixed)

RESOLVED FIXED
109 Branch
Tracking Status
firefox109 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

(Blocks 2 open bugs)

Details

Attachments

(5 files, 3 obsolete files)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review

For a while now, we have had a linter rule to validate that any idl constants used from Javascript are defined. This is currently run manually, and tends to pick up something that's broken once every couple of months.

We can't include this as part of the automatic ESLint, as it depends on the generated xpidl output, so would typcially require an opt build.

We would like to include this as a tier-2 task, that is run once a day. Picking up the failing patches is normally quite easy.

The basic process to run this would be:

  • Get the source.
  • Do an opt build, or get an opt build where we have the generated xpidl output.
  • Run ESLint with a special command that enables the extra rule.

:kats, I'd be interested in getting this over the line. I'm assuming searchfox has similar needs in terms of the xpt files - are these already exposed somewhere?

Flags: needinfo?(kats)

I'm unfamiliar with xpt files specifically, and I don't see any in searchfox. I do see some in my objdir/config/makefiles/xpidl/ folder - is that what you're referring to?

In general the searchfox task (e.g. this one) has a generated-files artifact that contains generated files that we want to index. But I don't see the xpt files in there, so maybe that's not useful to you. Searchfox also runs some sort of xpidl parsing directly, in this script but I suspect that is also not useful to you.

Flags: needinfo?(kats)

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2)

I'm unfamiliar with xpt files specifically, and I don't see any in searchfox. I do see some in my objdir/config/makefiles/xpidl/ folder - is that what you're referring to?

Yep.

In general the searchfox task (e.g. this one) has a generated-files artifact that contains generated files that we want to index. But I don't see the xpt files in there, so maybe that's not useful to you. Searchfox also runs some sort of xpidl parsing directly, in this script but I suspect that is also not useful to you.

Hm, yeah - but it would seem that we could maybe add something to https://searchfox.org/mozilla-central/rev/35d927df97900a57ecb562ad13909e392440b0fb/toolkit/mozapps/installer/packager.mk#77-93 to package up the xpt files so they too appear as artifacts on this job? With that it should be possible to do this, I think.

Mark, in comment #0 you explicitly mentioned opt builds, but afaict these are debug builds - does that make a difference for the linter? I wouldn't have thought so, but I'm probably missing something...

Flags: needinfo?(standard8)

Alternatively, I guess with a src dir only generating the xpidl xpt info and nothing else shouldn't actually be that difficult - the xpidl thing is just a python parser, IIRC?

(In reply to :Gijs (he/him) from comment #3)

Mark, in comment #0 you explicitly mentioned opt builds, but afaict these are debug builds - does that make a difference for the linter? I wouldn't have thought so, but I'm probably missing something...

Oh, I think I was thinking "opt" vs "artifact". I don't think it would really matter.

(In reply to :Gijs (he/him) from comment #4)

Alternatively, I guess with a src dir only generating the xpidl xpt info and nothing else shouldn't actually be that difficult - the xpidl thing is just a python parser, IIRC?

If we can do that, it'd probably be nice, easy and quick-ish.

Flags: needinfo?(standard8)
Depends on: 1479515
No longer blocks: 1479513
Depends on: 1479513
Assignee: nobody → standard8

Nick says he's thought about this a while ago.

Some more context about where we're at:

  • We now have an ESLint command that can be run locally with an optimised build:
    • MOZ_OBJDIR=objdir-ff-opt ./mach eslint --rule "mozilla/valid-services-property: error" --rule="mozilla/valid-ci-uses: error" *
  • We would like to get this run as a tier 2 builder on CI, so that new issues cause bugs to be filed for regressions.
  • For these rules to run, we need access to the xpt files in <objdir>/config/makefiles/xpidl/`

Hence I think we should package those xpt files either as part of the existing tests artifact, or as a separate artifact which is downloaded when necessary. If it is tests artifact, then we might be able to make these rules work locally for artifact builds as well.

Flags: needinfo?(nalexander)

This allows to check the executable mode of iterated entries.

For the specific case of .xpt artifacts, we could place them
directly into $topobjdir/config/makefiles/xpidl, but I'm wary of
modifying the object directory in this manner. I'd like the initial
round of tooling building on this capability to look in
dist/xpt_artifacts for artifact builds and be able to give detailed
feedback in error situations. We can revisit the placement of
artifacts in the future as more use cases and shortcomings are
identified.

In the future, this mechanism might be used to include Java code
generated at build-time that exposes IDL constants to GeckoView.

Depends on D161640

Mark: this seems to be working fine in automation; see, for example,

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

To test, use MOZ_ARTIFACT_TASK=<task ID> ./mach [build|artifact install] -v, etc.

As noted in the commit message, out of an abundance of caution (who owns what part of the object directory when can be subtle) and to ease implementation (there are many assumptions that artifacts are written to dist), the .xpt files are available in dist/xpt_artifacts. (I used xpt_artifacts as the easy-to-grep sentinel for this idea.) Tooling can either look there and fall back to $topobjdir/config/makefiles/xpidl, or recognize that this is an artifact build in some manner and look only in that location. As we grow usages, we can think more about populating the object directory directly.

Flags: needinfo?(nalexander) → needinfo?(standard8)
Attachment #9302616 - Attachment description: WIP: Bug 1666491 - Package `.xpt` artifacts for consumption by artifact builds. → Bug 1666491 - Package `.xpt` artifacts for consumption by artifact builds. r?#build
Attachment #9302617 - Attachment description: WIP: Bug 1666491 - Consume `.xpt` artifacts during artifact builds. → Bug 1666491 - Consume `.xpt` artifacts during artifact builds. r?#build

Here's a better try build:
https://treeherder.mozilla.org/jobs?repo=try&revision=3eb43f63b350ac8acc558162ddbf6ea085f6d4a0

I see target.xpt_artifacts.zip that looks sensible produced by a B job and the corresponding BA job has:

[task 2022-11-09T00:50:36.697Z] 00:50:36     INFO -  "/builds/worker/.mozbuild/package-frontend/4ac53502595d36ea-target.xpt_artifacts.zip" is a recognized extra archive (XPT Artifacts)
[task 2022-11-09T00:50:36.697Z] 00:50:36     INFO -  Adding xpt_artifacts/captivedetect.xpt to processed archive

I've not been able to get it to download locally (possibly as I'm on a different head?), but it looks good on try.

Would you also be able to help with getting a task running (or know who to ask)? I tried asking on CI on matrix, but didn't get a response.

I took a look, but it appears that we'd need a new mozharness test type along with associated scripts, to do basically the equivalent of https://searchfox.org/mozilla-central/rev/6f77213807eb5359c8afe458ac5628f973e92a25/taskcluster/ci/source-test/mozlint.yml#92-100, which seems a little excessive. So maybe I got something wrong there.

Flags: needinfo?(standard8) → needinfo?(nalexander)

On macOS, this allows the ResignJarWriter to handle ZIP (JAR) files
correctly.

The ESlint plugin can look for XPT artifacts:

  1. in the location specified by the environment variable
    MOZ_XPT_ARTIFACTS_DIR (for automation pretending to be a build);
  2. in dist/xpt_artifacts for builds known to be artifact builds;
  3. in $TOPOBJDIR/config/makefiles/xpidl for non-artifact builds.

Depends on D161641

It's possible that the processed file has not been created.

On macOS, this allows the ResignJarWriter to handle ZIP (JAR) files
correctly.

Depends on D162073

The ESlint plugin can look for XPT artifacts:

  1. in the location specified by the environment variable
    MOZ_XPT_ARTIFACTS_DIR (for automation pretending to be a build);
  2. in dist/xpt_artifacts for builds known to be artifact builds;
  3. in $TOPOBJDIR/config/makefiles/xpidl for non-artifact builds.

Depends on D161641

Attachment #9303430 - Attachment is obsolete: true
Attachment #9303429 - Attachment is obsolete: true
Attachment #9302615 - Attachment is obsolete: true
Blocks: 1800873
Blocks: 1800874

To help with making the landing simpler (not needing sheriff sign-offs etc), I'm going to do the initial landing as a tier 3, I'll use bug 1800874 to promote this up to a tier 2.

Flags: needinfo?(nalexander)
Summary: Create a tier-2 task for checking the use of idl constants to ensure they are valid → Create a tier-3 task for checking the use of idl constants to ensure they are valid
Attachment #9303427 - Attachment description: Bug 1666491 - Make new IDL constants task, depending on build; fetch XPT artifacts. r?Standard8 → 31;95;0cBug 1666491 - Add a eslint-build tester (tier 3) that depends on xpt artifacts. r?nalexander!,#releng-reviewers!
Summary: Create a tier-3 task for checking the use of idl constants to ensure they are valid → Create an eslint-build tester (tier 3) that depends on xpt artifacts - for checking the use of idl constants to ensure they are valid
Attachment #9303427 - Attachment description: 31;95;0cBug 1666491 - Add a eslint-build tester (tier 3) that depends on xpt artifacts. r?nalexander!,#releng-reviewers! → Bug 1666491 - Add a eslint-build tester (tier 3) that depends on xpt artifacts. r?nalexander!,#releng-reviewers!
Attachment #9302616 - Attachment description: Bug 1666491 - Package `.xpt` artifacts for consumption by artifact builds. r?#build → Bug 1666491 - Package `.xpt` artifacts for consumption by artifact builds. r?#build!
Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2bc50f7ab10a Pre: Delete failed (likely partially) processed artifacts. r=firefox-build-system-reviewers,glandium https://hg.mozilla.org/integration/autoland/rev/e7c65861b7ae Pre: Make `BufferedReader(JarFileReader)` work. r=firefox-build-system-reviewers,glandium https://hg.mozilla.org/integration/autoland/rev/dd5f6e80ff30 Package `.xpt` artifacts for consumption by artifact builds. r=firefox-build-system-reviewers,glandium https://hg.mozilla.org/integration/autoland/rev/a1e47fa15919 Consume `.xpt` artifacts during artifact builds. r=firefox-build-system-reviewers,glandium https://hg.mozilla.org/integration/autoland/rev/e20483a63756 Add a eslint-build tester (tier 3) that depends on xpt artifacts. r=nalexander,releng-reviewers,gbrown
Blocks: 1801283
Regressions: 1802260
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: