Zhou Zheng Sheng has posted comments on this change. Change subject: agent: Build and install ovirt guest agent on Ubuntu ......................................................................
Patch Set 3: (6 inline comments) First pass of my review, did not review files under debian. I need to digest Debian New Maintainers' Guide then review the second pass. There are some scripts duplicated in .spec file and debian/ , is it possible to extract the scripts out and have them shared by .spec and debian/ ? I googled a little and find .spec supports %include notation [1], maybe this can be useful. http://ramblingfoo.blogspot.com/2007/07/more-rpm-argh-or-how-to-use-includes.html .................................................... File Makefile.am Line 27: AUTHORS \ Line 28: README_Fedora.txt \ Line 29: ovirt-guest-agent.spec \ Line 30: m4/fhs.m4 \ Line 31: debian \ Looks like the original lines use spaces to indent, and the new lines use tabs. Line 32: $(NULL) Line 33: Line 34: # When fixing a file to conform with pep8 add it to the WL here so it will be Line 35: # checkd from now on .................................................... File ovirt-guest-agent/hibernate Line 1: #!/bin/bash I checked this script with "checkbashisms ovirt-guest-agent/hibernate", I found two trivial bashisms and we can change this script to sh. Line 2: Line 3: usage() { Line 4: echo "usage: $0 disk|mem" Line 5: exit 1 Line 6: } Line 7: Line 8: state="$1" Line 9: Line 10: if [ $state == "disk" ]; then Better to change "==" to "=" . Line 11: param="hibernate" Line 12: elif [ $state == "mem" ]; then Line 13: param="suspend" Line 14: else Line 7: Line 8: state="$1" Line 9: Line 10: if [ $state == "disk" ]; then Line 11: param="hibernate" Better to change "==" to "=" . Line 12: elif [ $state == "mem" ]; then Line 13: param="suspend" Line 14: else Line 15: usage .................................................... File ovirt-guest-agent/ovirt-guest-agent.in Line 1: #!/bin/bash I think this script is sh compatible. I checked it with checkbashisms, it just found some gettext problems, these are not harmful. Line 2: # Line 3: # ovirt-guest-agent - Startup script for the oVirt agent daemon Line 4: # Line 5: # chkconfig: 35 85 15 .................................................... File ovirt-guest-agent.spec Line 191: %exclude %{_datadir}/ovirt-guest-agent/GuestAgentWin32.py* Line 192: %exclude %{_datadir}/ovirt-guest-agent/OVirtGuestService.py* Line 193: %exclude %{_datadir}/ovirt-guest-agent/WinFile.py* Line 194: %exclude %{_datadir}/ovirt-guest-agent/setup.py* Line 195: %exclude %{_datadir}/ovirt-guest-agent/version.py* It's excluding Windows related files, is this related to this patch? Maybe it can be in another patch. Line 196: Line 197: %files pam-module Line 198: %{_moduledir}/pam_ovirt_cred.so Line 199: %exclude %{_moduledir}/pam_ovirt_cred.a -- To view, visit http://gerrit.ovirt.org/15313 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7a694bf1f7ff4230bac305dd571899265ef63950 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-guest-agent Gerrit-Branch: master Gerrit-Owner: Vinzenz Feenstra <[email protected]> Gerrit-Reviewer: Barak Azulay <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Vinzenz Feenstra <[email protected]> Gerrit-Reviewer: Zhou Zheng Sheng <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
