Closed Bug 1646427 Opened 4 months ago Closed 3 months ago

|mach vendor python| should exclude tests and docs

Categories

(Firefox Build System :: General, enhancement)

enhancement

Tracking

(firefox79 fixed)

RESOLVED FIXED
mozilla79
Tracking Status
firefox79 --- fixed

People

(Reporter: ahal, Assigned: ahal)

Details

Attachments

(4 files)

Currently |mach vendor python| will often include superfluous files like tests, docs, dotfiles, etc. Some of our vendored packages have pretty massive test directories.

I think in the past there was a bit of push back against removing them because they could theoretically be required by the package. But imo the lack of removal causes real pain here, and in the spirit of not letting perfect be the enemy of good, I'd like to simply exclude directories named "test", "tests" or "docs" (and then not worry about other stuff like the dotfiles).

In the rare event that a package actually needs one of the files in those directories, we can add a flag to disable that behaviour.

This adds a way to exclude certain file patterns when extracting Python
packages in |mach vendor python|. For now I've excluded likely test and doc
dirs, though we may want to expand on the list in the future.

Depends on D80208

Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1bafbdf97b9b
[mozfile] Convert 'test_extract.py' to the pytest format, r=gbrown
https://hg.mozilla.org/integration/autoland/rev/97e16bc7c604
[mozfile] Add ability to ignore file patterns while extracting, r=gbrown
https://hg.mozilla.org/integration/autoland/rev/eba33e59bc8f
[vendor] Exclude test and documentation dirs when vendoring Python packages, r=rstewart
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla79

There's a bug in moz-phab that has been preventing me from landing the last patch.. I'll ask a sheriff to land it for me if I can't figure out a work around.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed by ahalberstadt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b8c61efea3ce
[vendor] Re-run |mach vendor python| with the new ignore rules, r=rstewart

It looks like the most recent changeset added /third_party/python/pathspec/pathspec/tests/ which wasn't included before.

Yes, that was somewhat intentional.

I think the end goal here is that what we vendor should match what we specify in the ignore list so that developers running ./mach vendor python don't need to worry about it. In this case the developer that vendored pathspec manually removed that tests directory (I likely asked them to). But going forward, I think it's more important that the set of vendored files stays in sync with what is specified in the ignore list. Otherwise, running ./mach vendor python will cause unrelated side effects (i.e, constantly trying to add or remove stuff back in).

In the case of pathspec, it got added because pathspec places their tests directly in the actual package (pathspec/pathspect/tests instead of pathspec/tests) which imo is bad practice. Therefore our ignore rule of */tests doesn't match. To catch that tests directory as well we could change it to **/tests, but this feels a bit risky as there could be a legitimate directory called tests somewhere that would actually be needed by the package to function. So it felt safer to simply allow those files back in.

Status: REOPENED → RESOLVED
Closed: 3 months ago3 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.