[Distutils] Re: Distutil patch: clean command

[Greg Ward]

[cc'd to the distutils-sig, since this deserves a public airing]

On 13 March 2000, Bastian Kleineidam said:
> this patch is against the CVS version of March 13th.
> It implements a "clean" command which cleans everything
> generated with the "build" and "sdist" commands.

Useful addition, and definitely something I've been putting off.
However, there's no need to clean up after "sdist", since it
automatically does so itself!  (You can tell it not to with the
"--keep-tree" option.)  If "sdist" is *not* cleaning up after itself on
some platform, that's a bug.  (See the last statement of
'make_distribution()' in the "sdist" command class.)

> Additionally, I implemented the sdist option --list-only.

Procedural point: in future, please try to submit distinct changes like
this as separate patches.  I might like one change right away, but send
another back for revisions.  Bundling them together makes this hard.

As it happens, your "clean" command has a few problems, and I don't see
the point of the --list-only option.  (Partly my fault: I probably
should have taken it out when I revamped the "dist" command and turned
it into "sdist" -- I'm pretty sure it's vestigial.)

> + class clean(Command):
> + 
> +     # Hmm, split this up into clean_py,clean_ext,clean_clib,clean_sdist?
> +     # Or just clean_build,clean_sdist?

Good gravy!  No need to go overboard here; cleaning is simple enough
that it can be handled by one command.

> +     description = "clean everything we built"
> +     user_options = [
> +         ('build-base=', 'b',
> +          "base directory for build library"),
> +         ]
> + 
> +     def initialize_options(self):
> +         self.build_base = 'build'

No: 'initialize_options()' should almost always initialize everything to
None; otherwise, we have no way of knowing if the user supplied a value
when we get to 'finalize_options()'.

> +     def finalize_options(self):
> +         self.base_dir = self.distribution.name or "UNKNOWN"
> +         if self.distribution.version:
> +             self.base_dir = self.base_dir+"-"+self.distribution.version

Not necessary, since "sdist" cleans up after itself.  And it's *very*
naughty to generate a filename or directory name that's generated by
another command; if "sdist" doesn't provide a way to get that
information, add it (eg. a 'get_release_tree()' method).  But it's not
needed anyways.

But you do need to select a final value for 'build_base', and it should
come from the "build" command's 'build_base'.  Canonically:

    self.set_undefined_options ('build',
                                ('build_base', 'build_base'))

> +     def run(self):
> +         # remove the build directory
> +         self._nuke_tree(self.build_base)

Oh wait, there are two kinds of things I might want to clean in the
build base: temporary build by-products (foo.o, libbar.a, etc. -- all in
build/temp.<plat>) and the real products of the build (in build/lib or
build/platlib).  This probably should be a user option to the clean
command.  I'm not sure what the default should be: clean everything with
"--temp-only" option, or clean temp only with an "--all" option.  The
latter is safer and less typing in the "exceptional" case.

> +     def _nuke_tree(self, directory):
> +         try:
> +             self.execute (shutil.rmtree, (directory,),
> +                           "removing "+directory)
> +         except (IOError, OSError), exc:
> +             if exc.filename:
> +                 msg = "error removing %s: %s (%s)" % \
> +                        (directory, exc.strerror, exc.filename)
> +             else:
> +                 msg = "error removing %s: %s" % (directory, exc.strerror)
> +             self.warn (msg)

This looks suspiciously as though it was cut 'n pasted from the
'nuke_release_tree()' method in "sdist".  I hereby ban reuse by cut 'n
paste.  Since "nuke a directory tree" is now needed in two places, it
should be factored out into a convenience function in util.py.  (And it
should take 'verbose' and 'dry_run' flags... just follow the
well-established pattern.)

I'll happily accept a second patch!  Just keep it distinct from any
other hacks.

Now the "sdist" business...

Why is the "--list-only" option needed?  I'm pretty sure it should be
dropped, because you can achieve everything it provided (and more) with
the new "sdist" command.  Eg:

   setup.py sdist --manifest-only

generates MANIFEST from MANIFEST.in, giving you the list of files that
will be distributed.  It even tells you what's going on as it processes
the MANIFEST.in file; right now this is debugging output, but I'm
starting to think it should be preserved.  It's handy!

   setup.py -n sdist --manifest-only

This does the same, only it doesn't create MANIFEST -- just tells you
what it would have done while processing MANIFEST.in.

    setup.py -n sdist

And this tells you what "sdist" would have done in creating a
distributable archive (tarball or zip or whatever).

Is anything else needed?  Does your patch (which I haven't read closely) 
provide anything extra that's not handled by these options?

        Greg
-- 
Greg Ward - "always the quiet one"                      gward@python.net
http://starship.python.net/~gward/
Think honk if you're a telepath.