Closed Bug 1184696 Opened 7 years ago Closed 6 years ago

Easily purge compiled python files

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox43 fixed)

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

Details

Attachments

(1 file)

We've had at least two instances of leftover .pyc files from deleted or moved python modules causing import failures.

`hg purge` doesn't remove them because they're in hgignore. `hg purge --all` would also clobber any objdirs that live under srcdir.

We should implement |mach purge| that works with both hg and git. Maybe it could work with file path globbing like |mach purge **.pyc|.
Also I think bug 1184625 would have never happened if we had used relative imports in marionette - though this is only possible if mozharness (and mach) does not try to run marionette using a file inside the package. :)

See 983821 comment 7.
Could `mach clobber --all` or `mach clobber --purge` accomplish the same thing? I'm somewhat sensitive to new commands that have strong overlap with existing ones. But otherwise, big +1 for this feature.
Moving this to Build System, because that's where to tend to put these types of issues, for lacking of a better component.
Component: General → Build Config
Product: Developer Services → Core
(Of course, it would be even better if the .pyc files weren't created in the source tree in the first place </broken_record>)
(In reply to Mike Hommey [:glandium] from comment #5)
> (Of course, it would be even better if the .pyc files weren't created in the
> source tree in the first place </broken_record>)

We could add 'sys.dont_write_bytecode' to the mach bootstrap, then at least they wouldn't get generated when running any mach commands:
https://docs.python.org/2/library/sys.html#sys.dont_write_bytecode

(In reply to Gregory Szorc [:gps] from comment #3)
> Could `mach clobber --all` or `mach clobber --purge` accomplish the same
> thing? I'm somewhat sensitive to new commands that have strong overlap with
> existing ones.

I'd be fine with this, I thought you would be the one who might object to it because it's currently tied to the active objdir. There would be a some ux to work out though, e.g does clobber --all remove all objdirs? What if you only want to remove compiled python and not your objdir?
If the only thing we care about purging is compiled python, another option might be |mach python --purge|
pyc files are necessary to achieve optimal build system performance. Somewhere there is a bug for writing a custom Python importer that won't write .pyc files in the source directory. If it were easy, it would have been done already. I've attempted to go down this road at least 3 times in the past 3 years and it isn't fun. The frequency of .pyc files biting us doesn't seem to justify the time required to fix the problem.
I experimented a bit. Shelling out to the underlying vcs is much faster than walking the file system. I used the following commands:
hg purge --all -I "glob:**/*.py[co]"
git clean -f -x "*.py[co]"

I'll put up a patch that just throws those into |mach python --purge| for now as that has the smallest footprint.
Bug 1184696 - Add |mach python --purge| for removing .pyc and .pyo files in topsrcdir, r=gps
Attachment #8636148 - Flags: review?(gps)
Comment on attachment 8636148 [details]
MozReview Request: Bug 1184696 - Add clobber targets to |mach clobber|; Ability to clobber compiled python files, r=gps

https://reviewboard.mozilla.org/r/13639/#review12257

::: python/mach_commands.py:38
(Diff revision 1)
> +        cmd = ['hg', 'purge', '--all', '-I', 'glob:**/*.py[co]']
> +    else:
> +        cmd = ['git', 'clean', '-f', '-x', '*.py[co]']

A problem with this regular expression is that .py files that haven't been put under version control yet will be deleted. It's better to explicitly use *.pyc and *.pyo.

::: python/mach_commands.py:39
(Diff revision 1)
> +    else:

You should explicitly look for .git. In rare cases, the user may not be using version control. Or some special snowflake may be using yet another VCS tool.

::: python/mach_commands.py:49
(Diff revision 1)
> -    def python(self, args):
> +    @CommandArgument('--purge',
> +        default=False,
> +        action='store_true',
> +        help='Purge all .pyc and .pyo files from topsrcdir and topobjdir')
> +    def python(self, args, purge):
> +        if purge:
> +            return purge_compiled_python(self.topsrcdir)

I'm not a fan of overloading `mach python`. I thought you were going to add this to `mach clobber`?
Attachment #8636148 - Flags: review?(gps)
Duplicate of this bug: 1010603
Sorry, put this on the shelf for a bit..

(In reply to Gregory Szorc [:gps] from comment #11)
> I'm not a fan of overloading `mach python`. I thought you were going to add
> this to `mach clobber`?

I can add it to |mach clobber|. It's a little unclear to me what |mach clobber --purge| should do though. Should it only clobber .pyo and .pyc files? Should it clobber everything except the objdirs? Should it clobber everything including the objdirs? I have an idea, patch coming soon.
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
(In reply to Gregory Szorc [:gps] from comment #11) 
> A problem with this regular expression is that .py files that haven't been
> put under version control yet will be deleted. It's better to explicitly use
> *.pyc and *.pyo.

Is this shell dependent? It doesn't delete untracked .py files for me.
Note that bug 1188224 makes this less pressing.
Comment on attachment 8636148 [details]
MozReview Request: Bug 1184696 - Add clobber targets to |mach clobber|; Ability to clobber compiled python files, r=gps

Bug 1184696 - Add clobber targets to |mach clobber|; Ability to clobber compiled python files, r=gps

This change adds a 'what' parameter to |mach clobber| which facilitates clobbering other things as well.
E.g, |mach clobber python| now clobbers all .pyc and .pyo files in the tree. Multiple clobber targets can
be specified, e.g |mach clobber objdir python|. By default |mach clobber| without arguments will only
remove the currently active objdir, the same as before.
Attachment #8636148 - Attachment description: MozReview Request: Bug 1184696 - Add |mach python --purge| for removing .pyc and .pyo files in topsrcdir, r=gps → MozReview Request: Bug 1184696 - Add clobber targets to |mach clobber|; Ability to clobber compiled python files, r=gps
Attachment #8636148 - Flags: review?(gps)
(In reply to Mike Hommey [:glandium] from comment #15)
> Note that bug 1188224 makes this less pressing.

Ah cool, I didn't know about this. I had the patch almost finished, so uploaded anyway. I'm fine WONTFIX'ing this if it isn't worth it anymore.
https://reviewboard.mozilla.org/r/13639/#review12257

> A problem with this regular expression is that .py files that haven't been put under version control yet will be deleted. It's better to explicitly use *.pyc and *.pyo.

This doesn't appear to be the case for me, untracked .py files were not deleted. Is this platform dependent?
https://reviewboard.mozilla.org/r/13639/#review12257

> This doesn't appear to be the case for me, untracked .py files were not deleted. Is this platform dependent?

regexp fail on my part. [co] means "one of 'c' or 'o'." I was interpretting it as "[co]?", which is "optionally one of 'c' or 'o'."
Comment on attachment 8636148 [details]
MozReview Request: Bug 1184696 - Add clobber targets to |mach clobber|; Ability to clobber compiled python files, r=gps

https://reviewboard.mozilla.org/r/13639/#review13951

Looks good.

Although, I posit the questions: is there any harm in deleting .py[co] files by default?
Attachment #8636148 - Flags: review?(gps) → review+
Comment on attachment 8636148 [details]
MozReview Request: Bug 1184696 - Add clobber targets to |mach clobber|; Ability to clobber compiled python files, r=gps

https://reviewboard.mozilla.org/r/13639/#review13957

::: python/mozbuild/mozbuild/mach_commands.py:549
(Diff revision 2)
> +                cmd = ['hg', 'purge', '--all', '-I', 'glob:**/*.py[co]']

Oh, this should be `glob:**.py[co]`. Per `hg help patterns`:

   Glob examples:

      glob:*.c       any name ending in ".c" in the current directory
      *.c            any name ending in ".c" in the current directory
      **.c           any name ending in ".c" in any subdirectory of the
                     current directory including itself.
      foo/*.c        any name ending in ".c" in the directory foo
      foo/**.c       any name ending in ".c" in any subdirectory of foo
                     including itself.
Attachment #8636148 - Flags: review+
(In reply to Gregory Szorc [:gps] from comment #20)
> Although, I posit the questions: is there any harm in deleting .py[co] files
> by default?

What do you mean by default, isn't that what bug 1188224 already does? It takes a non-trivial amount of time to run hg purge, so I'd avoid doing unnecessarily.

With bug 1188224 being landed, the main benefit here is that we can tell people to try running |mach clobber python| if they come to us with wonky python behaviour. In the future, we might also want to clobber virtualenvs as part of this.
Comment on attachment 8636148 [details]
MozReview Request: Bug 1184696 - Add clobber targets to |mach clobber|; Ability to clobber compiled python files, r=gps

Bug 1184696 - Add clobber targets to |mach clobber|; Ability to clobber compiled python files, r=gps

This change adds a 'what' parameter to |mach clobber| which facilitates clobbering other things as well.
E.g, |mach clobber python| now clobbers all .pyc and .pyo files in the tree. Multiple clobber targets can
be specified, e.g |mach clobber objdir python|. By default |mach clobber| without arguments will only
remove the currently active objdir, the same as before.
Attachment #8636148 - Flags: review?(gps)
Comment on attachment 8636148 [details]
MozReview Request: Bug 1184696 - Add clobber targets to |mach clobber|; Ability to clobber compiled python files, r=gps

https://reviewboard.mozilla.org/r/13639/#review13961

Ship It!
Attachment #8636148 - Flags: review?(gps) → review+
https://hg.mozilla.org/mozilla-central/rev/0f8675867f52
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.