Shawn Walker wrote: > 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?
Yes, I wanted to separate the cli and the gui for the first inclusion, but this can be changed. So to actually have svr4 packages for the gui and cli you need to type: make make gui-install make install > 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. I didn't have such problem. > 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 Changed > > 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. > This is also changed > 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. This is fine, those are set from the source code, those numbers are only for the test version of the gui. > http://cr.opensolaris.org/~migi/packagemanager-integration/src/cmd/gui/modules/thread.py.html > ============================== > 39 # raise CancelThreadException, "Thread canceled" > Changed > 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. That seems wrong to me, how the app should know if the user have rights to install/remove packages? This was the easiest way of doing this without checking user profiles/roles. If the user doesn't have rights all the buttons should be disabled. In the above example it is possible to determine if the user have rights after performing operation such as install/remove/update and then disabling buttons... ? > 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? > I have made some changes, not sure if it is fine for you now. > 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? I had a lot of trouble passing integration scripts and those were mandatory fields required by the script, so that is why I've added them. > === > > It would be good to run pylint over these files; I did and found quite > a few possible things that should be fixed. I have made many changes in the formatting so it should now be similar to the IPS. best Michal _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
