Dan,
Thank you for the comments. Please see inline for responses.
>
> Some comments:
>
> When you are writing new test cases, as in t_plat.py, please
> ensure they all have docstrings.
>
> Couldn't you make use of assertRaises for some of the t_play.py tests?
>
> 43 def testGroup(self):
> 44 if os.path.exists("/etc/group"):
> 45 try:
> 46 portable.get_group_by_name("ThisShouldNotExist", "/",
> True)
> 47 self.fail("get_group_by_name returned something")
> 48 except KeyError:
> 49 pass
> 50 try:
> 51 portable.get_name_by_gid(87285, "/", True)
> 52 self.fail("get_name_by_gid returned something")
> 53 except KeyError:
> 54 pass
>
> Can this be recoded as follows:
>
> if os.path.exists("/etc/group"):
> self.assertRaises(KeyError, portable.get_group_by_name,
> "ThisShouldNotExist", "/", True)
>
> self.assertRaises(KeyError, portable.get_name_by_gid, 87285,
> "/", True)
>
> Same for the rest of the test cases.
>
Fixed.
> t_commandline.py: I don't understand this change.
>
In client.py, there is a call to image.find_root and image.load_config
before the subcommand arguments are processed. If the the current
directory is not within an image, then the pkg command fails with a
different return code than what is expected by the test. When running
the test cases on Solaris, every directory is within an image because
the /var/pkg directory is there. So one never sees this problem on
Solaris. But on other systems such as Windows or Linux, there is no
/var/pkg directory, so this problem shows up. Adding the code to create
an image ensures that the commands being test run within an image so
that the right error cases actually happen.
> t_pkg_install_basics: I thought Bart had putback a sort of "sample image"
> with a stubbed out /etc/passwd, etc. I'd prefer these files to be set up
> in the gate, and then referenced from there, instead of being generated on
> the fly. That means we can reuse them in many different test cases, and
> don't need to worry about cleaning them up.
>
> That same technique would also apply to t_pkgsend.py and other test cases.
>
Yes, there are some files under the tests/cli/testdata directory. For
the t_pkg_install_basics test, I borrowed code from the t_upgrade test.
Before, the t_pkg_install_basics test was depending on files such as
/lib/libc.so.1 that are not available on all platforms. So I modified
the test to create its own files.
If you prefer, I can certainly put these test files into
tests/cli/testdata instead and check them in. Should I change both the
t_pkg_install_basics and t_upgrade tests?
> You could avoid the hassle of updating harnesslib, etc. if you migrated
> two_depot.ksh to python. :) I just haven't gotten around to it yet.
>
> Can you give us some sample pylint output?
>
I copied the pylint output here:
http://cr.opensolaris.org/~tmueller/pylint-output.txt
This output is from:
python setup.py lint > pylint-output.txt 2>&1
It still produces way too many messages. This is intended as a start
that will need to be refined over time.
I didn't make any attempt to fix lint messages in files that I didn't
otherwise touch.
> portable/__init__.py:
>
> Please delete all of the
>
> 56 #####################################################################
> 57 ## Platform Methods
> 58 #####################################################################
>
> This kind of stuff is not in keeping with the project's coding style.
>
Fixed.
> # Platform Methods
> # ----------------
>
> Would be acceptable. Or simply use vertical whitespace.
>
> 59 # get_isainfo() - Return the information for the OS's supported ISAs.
> 60 # This can be a list or a single string.
> 61 # get_release() - Return the information for the OS's release version.
> This
> 62 # must be a dot-separated set of integers (i.e. no alphabetic
> 63 # or punctuation).
> 64 # get_platform() - Return a string representing the current hardware
> model
> 65 # information, e.g. "i86pc".
>
> Add vertical whitespace between these, please. It only costs one byte.
>
>
Fixed.
>
> 28 +# NOTE: This module is inherently Unix specific. It does not work on
> Windows.
> 29 +# Care is taken in the modules that use this module to not use it on
> Windows.
> 30 +
>
> Unix is spelled: UNIX. It would be nice to simply say:
>
> NOTE: This module is inherently UNIX specific; Care is taken in the
> modules
> that use this module to not use it on other operating systems.
>
> Perhaps "unix" should really be "posix"?
>
>
Fixed.
> image.py:
>
> 686 + def get_user_by_name(self, name):
> 687 + return portable.get_user_by_name(name, self.root,
> 688 + self.type !=
> IMG_USER)
> 688 689
> 689 - Keep a cached copy in memory for fast lookups, and
> fall back to
> 690 - the current environment if the password database
> isn't
> 691 - available.
> 692 - """
> 690 + def get_name_by_uid(self, uid):
> 691 + return portable.get_name_by_uid(uid, self.root,
> 692 + self.type !=
> IMG_USER)
> 693 693
> 694 - # XXX What to do about IMG_PARTIAL?
> 695 - if self.type == IMG_USER:
> 696 - return pwd.getpwnam(name)
> 694 + def get_group_by_name(self, name):
> 695 + return portable.get_group_by_name(name, self.root,
> 696 + self.type !=
> IMG_USER)
> 697 697
> 698 - passwd_file = os.path.join(self.root, "etc/passwd")
> 698 + def get_name_by_gid(self, gid):
> 699 + return portable.get_name_by_gid(gid, self.root,
> 700 + self.type !=
> IMG_USER)
>
> Why throw away the image type and turn it into a boolean? What if we need to
> extend
> this logic to deal with other things?
The portable module does not depend on the image module. If the image
type were passed in, then portable would need to import image in order
to make sense out of the value. These methods are designed to only deal
with things at the passwd and group file level. It is up to the image
module to determine how this relates to image types.
> Please note that you deleted the
>
> # XXX What to do about IMG_PARTIAL?
>
> But didn't resolve it.
>
>
I added this comment back into the image.py file.
> catalog.py:
>
> 1174 - usec = int(ts[20:26])
> 1185 + # usec is not in the string if 0
> 1186 + if len(ts) > 19 and ts[19] == ".":
> 1187 + usec = int(ts[20:26])
> 1188 + else:
> 1189 + usec = 0
>
> Seems like it might be easier to do:
>
> try:
> usec = int(ts[20:26])
> except ValueError:
> usec = 0
>
>
Yes, that is better. Fixed.
> ---
> I see that you've neutered certain actions. I'm not sure that I particularly
> like this. Why not simply filter out actions you don't want?
>
Are you referring to group and user?
By filtering out such actions, are you suggesting that something be
built into the imageplan or pkgplan that would automatically filter out
actions depending on the environment? This isn't just an issue relating
to portability as this also comes up when dealing with user images. For
example, installing a device driver doesn't make much sense for a
non-root user working with a user image.
This is an interesting idea. Can we defer this for a later checkin though?
Thanks.
Tom
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
