2009/6/30 Paul Moore <p.f.mo...@gmail.com>:
> Thank you. I'll try to make the time to go through the PEP and comment
> more fully.
OK, I've now read the PEP through in full. My comments follow. I have
deliberately avoided mentioning points that others have already
raised, to keep things shorter.
First of all, after I got over the use of setuptools terminology, the
document is very good. It's clear what it's trying to achieve, and
covers most of the points I'd want it to. Occasionally the
presentation is confusing, see below, but I think everything was
there.
I do still feel that the setuptools focus will put some readers off.
The introductory remarks, in particular, assume a reasonable level of
familiarity from readers about setuptools, easy_install, pip and the
like. Not having that familiarity isn't a problem in fact, but the
assumption is there in the tone. (For example, personally, I hate the
unnecessarily cute term "egg", but it's established by now, so that's
a lost cause :-()
Onto the PEP.
"The problem is that many people use easy_install or pip to install
their packages..."
As already noted by others, it's not clear why this is a problem. But
worse is the fact that the paragraph reads to me as saying that
easy_install/pip/etc don't follow the *current* standard structure.
Given this fact (which *is* a problem!) I fail to see why I should
expect them to follow any *new* standard - so how does this PEP
actually address the issue? Rather, it seems to say to me that the
existing tools have a history of ignoring the standard approach, so
this PEP is going to be useless :-(
[I expect that isn't what you're trying to say, so you may just need
to clarify your meaning. But I do think it's important to address the
question of how this PEP is going to ensure that existing tools adapt.
In particular, setuptools seems to have completely stagnated, so I see
very little likelihood that setuptools is going to change to conform
to the PEP - how does that affect things?]
".egg-info becomes a directory"
Don't refer to the setuptools documentation (EggFormats)! It is
obscure and user-hostile, as well as not actually being the same as
the PEP's proposal. Rather, just document the structure of .egg-info.
If you want, you can then add a cross-reference note, saying something
like "The setuptools structure, as proposed in the EggFormats
documentation for that package [ref], is a subset of this standard. In
order to conform to this PEP, setuptools will have to be amended to
only install .egg-info directories in the format defined by this PEP".
"However, it will impact the setuptools and pip projects, but given
the fact that..."
Confusing. Will these tools need to change (I believe so) or not? If
they will need to change, that hardly counts as "no deep consequences"
- there's the whole backward compatibility issue for them to handle.
"Adding a RECORD file"
You say "at installation time" - please clarify. Do you only mean
setup.py install? What about bdist_wininst and bdist_msi? What about
third party bdist style tools? How will they ensure they get a RECORD
file?
"The RECORD format"
The line separator shouldn't be os-dependent. What value is used for a
pure-python (ie, platform independent) package? Unless the file is
generated when the install is done, in which case see the previous
point...
Absolute file paths - this implies that RECORD has to be generated by
the installer (which is the only place that knows absolute paths)
which means that every bdist_xxx installer has to write its own RECORD
file. Does the PEP offer no support for this? In any case, the
bdist_wininst and bdist_msi code (which is core distutils) will need
to be amended to manage RECORD files appropriately.
"Adding an INSTALLER file in the .egg-info directory"
Same question again - does the PEP offer supporting APIs to help
bdist_xxx installers do this? What about the core bdist_xxx commands?
"New APIs in pkgutil"
You say "the best place to put these APIS seems to be pkgutil". You
should be more definite - "these APIs will be added to the pkgutil
module".
The Distribution/ZippedDistribution and
DistributionDir/ZippedDistributionDir pairs seem to imply that users
need to explicitly instantiate these classes (and hence know whether a
distribution is zipped). This is cleared up later, but you should add
a note here - "Users will not need to create instances of these
classes manually, they are returned by the public functions in
pkgutil, such as get_distributions()".
"DistributionDirMap class"
This seems to be an internal implementation detail, and as such should
not be documented. It's never returned from any of the public APIs,
and the only created instance is a hidden internal global instance.
The whole part about purging the cache is also unnecessary - the user
has no interface to allow them to do this, so you don't need to
document that it's possible.
Actually, (Zipped)DistributionDir instances are never returned to the
user via the public API, either - so these don't need to be
documented.
General rule - don't document (and commit yourself to) any internal
details that the user can't access from the public API. It just makes
backward compatibility harder to maintain.
"Adding an uninstall function"
With regard to my earlier comments, my apologies - I misread the text
and assumed the documented function simply returned the files to
remove, but didn't do the removal itself. (Which makes it far more
useless than it actually is).
One point - what happens if the uninstall method can't remove a file
or directory? Will it leave things in a broken/partially installed
state? This needs to be documented (although huge robustness and
recovery measures don't need to be taken in a simple reference
implementation). This is likely to be a somewhat common issue on
Windows in particular, where you can't delete open files.
"Installer marker"
There are times when I would imagine it would be useful to force
uninstallation of the basic package, even if that leaves stuff behind
related to a particular installer. A force=True argument would be the
easiest, but if you'd rather not add that, it would be useful to
document the workaround:
inst = get_distribution('docutils').get_egginfo_file('INSTALLER').read()
uninstall('docutils', installer=inst)
If nothing else, it shows a simple example of how to use the APIs. A
few more such examples in the PEP might be nice.
But as I said, overall, I'm much happier with the PEP now that I've
read it through and taken the time to digest it.
Paul.
_______________________________________________
Python-Dev mailing list
Python-Dev@python.org
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe:
http://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
