2008/5/20 Michal Pryc <[EMAIL PROTECTED]>: > Hello, > We have been discussing putting packagemanager code into the pkg tree: > http://mail.opensolaris.org/pipermail/pkg-discuss/2008-April/002780.html > > I have made changes which are available for review at: > http://cr.opensolaris.org/~migi/packagemanager-integration/ > > This version of packagemanager is cleaned up source code, so only > working and integrated bits are there without any additional files which > are in the old repository. > > As we were discussing there is the following structure: > src > cmd > gui > > > So there is a space for: > src > cmd > cli > > I didn't touch anything from the current sources except one file > src/Makefile > > The following make commands works with the gui: > > make (this builds *pyc files for both cli and gui) > make install (this creates only cli package SUNWipkg) > make gui-install (this creates SUNWipkg-gui and SUNWipkg-gui-data) > make clean (cleans all source code together with gui) > > Any comments welcome
"make install" in the top-level src/ directory, doesn't perform a gui-install. Is this intentional? http://cr.opensolaris.org/~migi/packagemanager-integration/src/cmd/gui/Makefile.html ============================== Once I did "make install", I then switched to src/cmd/gui and performed "make install" and received this error: rm -f ../../../gui-proto/root_i386/share/package-manager/modules/packagemanager.glade; install -f ../../../gui-proto/root_i386/share/package-manager -m 0444 modules/packagemanager.glade ../../../gui-proto/root_i386/share/package-manager/packagemanager.glade: Permission denied *** Error code 1 make: Fatal error: Command failed for target `../../../gui-proto/root_i386/share/package-manager/modules/packagemanager.glade' Checking further, I can see that the directory: ../../../gui-proto/root_i386/share/package-manager/modules/ ...doesn't exist. In short, there seems to be something wrong with the install target for the gui. http://cr.opensolaris.org/~migi/packagemanager-integration/src/cmd/gui/modules/__init__.py.html ============================== Your CDDL comments are done assigning the license text to a variable. The other package source code uses a comment block instead which seems less wasteful. Any thoughts on changing this? Line 33: Commas are not followed by a space http://cr.opensolaris.org/~migi/packagemanager-integration/src/cmd/gui/modules/enumerations.py.html ============================== Some of the comments here have a space after the '#' mark, some don't. http://cr.opensolaris.org/~migi/packagemanager-integration/src/cmd/gui/modules/installupdate.py.html ============================== There are several bits of commented code in here that look like they should be removed. Lines 103, 230, 259, 260, 263, 269, 270, 274, 275, 278, 284, 285, 288, 289, 474, and 502. 239 def installation_stage(self): ... It looks like the logic in here could be condensed quite a bit as it is nearly identical between update and install. 548 for package_plan in self.ip.pkg_plans:... ... The code in here could use a bit of commentary, and it looks like it could be simplified since the install and update cases are so similar. http://cr.opensolaris.org/~migi/packagemanager-integration/src/cmd/gui/modules/packagemanager.glade.html ============================== 816 <property name="label" translatable="yes">6 packages will be installed 817 28 packages will be updated 818 819 20.1 MB will be downloaded</property> The above doesn't seem right to me. http://cr.opensolaris.org/~migi/packagemanager-integration/src/cmd/gui/modules/thread.py.html ============================== 39 # raise CancelThreadException, "Thread canceled" http://cr.opensolaris.org/~migi/packagemanager-integration/src/cmd/gui/modules/userrights.py.html ============================== The docstrings for the functions in this file are missing punctuation at the end of each paragraph. 46 def check_administrative_rights(self): The check done here for Solaris doesn't seem right to me. The last time I asked a Sun engineer about how "permission checks" should be handled, I was told that an application should simply perform operations and fail gracefully. You shouldn't "check for permission" first before attempting. http://cr.opensolaris.org/~migi/packagemanager-integration/src/cmd/gui/packagemanager.py.html ============================== 201 def init_tree_views(self): The code in here is a bit jumbled together (until line 289). Can you break this up into commented block sections? http://cr.opensolaris.org/~migi/packagemanager-integration/src/cmd/gui/pkgdefs/SUNWipkg-gui-data/pkginfo.html http://cr.opensolaris.org/~migi/packagemanager-integration/src/cmd/gui/pkgdefs/SUNWipkg-gui/pkginfo.html ============================== 27 ARCH=i386 Should this be "ISA" ? 38 MAXINST=1000 Is this really needed? === It would be good to run pylint over these files; I did and found quite a few possible things that should be fixed. As for the structure used to integrate the changes, it looks fine to me. Cheers, -- Shawn Walker _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
