Re: Distutil patch: clean command
[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.
participants (1)
-
Greg Ward