Closed Bug 1433522 Opened 6 years ago Closed 6 years ago

Annotate 3rd party paths with files metadata

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: ahal, Unassigned)

References

Details

There's currently a list of third party paths under:
tools/rewriting/ThirdPartyPaths.txt

But this lists individual modules explicitly, rather than the minimum set of paths that need to be excluded (e.g third_party/python/six instead of third_party/**). This means when new modules are added, it's easy for this to become out of date.

Instead, I'd like to use moz.build to annotate 3rd party files. I'd propose something like:

with Files('third_party/**'):
    THIRD_PARTY = True

Once we have this, we can get tools like |mach lint| and |mach static-analysis| to query 3rd party code and automatically ignore it. I'd assume this would also be useful for code coverage.
i like this idea, but i'd love to see a fix for identifying 3rd party code that is clear to new contributors.

eg. if we moved _all_ third-party code under the third_party directory, this trivialises the problem of identifying this code, for both tooling and people.
I agree that in general we should put as much third party code as possible under /third_party. However, unless we do reach 100%, we'll still need some kind of system like the one this bug proposes. Considering:

1) Some third_party code might make sense to live elsewhere for packaging reasons
2) Some developers will want the third_party code to live next to the modules that use it
3) We likely have duplicates of third_party code at different versions
4) The sheer amount of third_party code

I don't think 100% will ever be attained :(

With this metadata though, there's certainly things we can do to make it easier for contributors and employees alike.
Hm, maybe a simple list in a moz.build variable is a better approach than using files metadata. Something like:

THIRD_PARTY_PATHS += ['third_party', 'servo', 'nsprpub']

I think really the only thing we care about is minimum set of paths that contain all 3rd party code. Calculating this using files metadata is going to be prohibitively expensive (unless I'm doing it wrong), whereas a moz.build list will be cheap.
Gps, do you have thoughts on the better approach (re: comment 3)?
Flags: needinfo?(gps)
I'm pretty sure not all third party code is restricted to particular directories - if you look through .eslintignore I'm sure there's some documented examples there.
(In reply to Andrew Halberstadt [:ahal] from comment #2)
> I agree that in general we should put as much third party code as possible
> under /third_party. However, unless we do reach 100%, we'll still need some
> kind of system like the one this bug proposes.

i agree.

> I don't think 100% will ever be attained :(

i disagree :)  i don't think any of your points are show-stoppers.  but that's a story for a different bug i suspect.

(In reply to Mark Banner (:standard8) from comment #5)
> I'm pretty sure not all third party code is restricted to particular
> directories - if you look through .eslintignore I'm sure there's some
> documented examples there.

it's all over the repo; see the file that ahal referenced in comment 0 (which are exclusions for the linting system): https://hg.mozilla.org/mozilla-central/file/tip/tools/rewriting/ThirdPartyPaths.txt
I like Files() because adding annotations to tracked files in the repo is the whole point of Files(). There is a larger discussion brewing about whether to keep Files() in moz.build or to extract the functionality to e.g. standalone YAML files. But for now, I think the proposal in comment #0 is reasonable.
Flags: needinfo?(gps)
Sorry, think I did a poor job explaining the problem with the proposal in comment 0. Let's say we have this directory structure:

root
  - third_dir
    - foo
    - bar
  - dir
    - baz
    - third_file

The goal is to generate this list of paths:
- root/third_dir
- root/dir/third_file

If the metadata is attached to each file, doing this involves:
1) Reading metadata from every file in-tree
2) Collapsing that down to a minimum set of paths such that no 1st party files are contained inside

This is far too expensive to calculate at runtime (i.e when running |mach lint|), the bugzilla components task takes 20 minutes just to iterate over everything. So we'd need a task that generates an artifact of paths to exclude, though anytime this artifact needs to be re-generated the lint command will take 20+ minutes longer to complete.

Maybe this approach is still the right one though. Maybe a flat list of paths in moz.build would also be too expensive to process at runtime and we might as well just use Files as it is more flexible.
I spent a little bit more time looking into this and I'm now fairly convinced that the proposal in comment 0 is the wrong way to do this. Here is my new proposal.

The most analogous thing that already exists in moz.build to what we're trying to accomplish here are the SPHINX_TREES/SPHINX_PYTHON_PACKAGE_DIRS variables. Namely both those and THIRD_PARTY_PATHS are variables that exist solely in moz.build to be consumed by external tools (no processing done by the build system).

The sphinx variables are pretty heavily special cased:
https://dxr.mozilla.org/mozilla-central/rev/dc70d241f90df43505ece5ac12261339e9694c50/python/mozbuild/mozbuild/frontend/reader.py#925

I propose we make that function more generic (read an arbitrary input list of variables), and calculate THIRD_PARTY_PATHS at |mach lint| runtime. Turns out parsing moz.build's ast that way is really fast (I guess that's why it was implemented like that in the first place :))
(In reply to Byron Jones ‹:glob› from comment #6)
> > I don't think 100% will ever be attained :(
> 
> i disagree :)  i don't think any of your points are show-stoppers.  but
> that's a story for a different bug i suspect.

Just FWIW, consider that NSPR and NSS are both projects that get vendored from other repositories into m-c (albeit they come from other hg.mo repositories). I'm not sure we'd ever get sufficient buy-in to move them to third_party.

There's a previous attempt at this in bug 1130343.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #10)
> (In reply to Byron Jones ‹:glob› from comment #6)
> > > I don't think 100% will ever be attained :(
> > 
> > i disagree :)  i don't think any of your points are show-stoppers.  but
> > that's a story for a different bug i suspect.
> 
> Just FWIW, consider that NSPR and NSS are both projects that get vendored
> from other repositories into m-c (albeit they come from other hg.mo
> repositories). I'm not sure we'd ever get sufficient buy-in to move them to
> third_party.

we don't need to get buy-in from everyone; we need consensus from firefox stakeholders that a change would be overall beneficial to firefox as a whole.
Product: Core → Firefox Build System
Given your dev-platform proposal do you want to WONTFIX this? Or is this a step in your plan.
Flags: needinfo?(glob)
(In reply to Andrew Halberstadt [:ahal] from comment #12)
> Given your dev-platform proposal do you want to WONTFIX this? Or is this a
> step in your plan.

looks like things have settled down on dev-platform around my proposal, with general consensus that it's a good idea.
using bug 1454867 to track the work.
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(glob)
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.