Juan Hernandez has posted comments on this change.
Change subject: ShellBox UI plugin - rpmbuild
......................................................................
Patch Set 1: (8 inline comments)
Some comments inside the .spec.
I would suggest to move the .spec file to the root directory of the plugin
source, no need for the rpmbuild/* directories. Those are needed when building
the .rpm, but they shouldn't exist in the repository.
The .rpm and .tar files don't belong into the repository either.
I would also suggest to include some reference to ovirt in the name of the
package, something like ovirt-engine-shellinabox-plugin.
....................................................
File shellbox-plugin/rpmbuild/SPECS/shellbox-uiplugin.spec
Line 4: Release: 1
Line 5: License: GPL
Line 6: BuildArch: noarch
Line 7: Source0: %{name}-%{version}.tar
Line 8: BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root
It is better to avoid this BuildRoot line, the RPM build system already
generates a buildroot temporary directory automatically.
Line 9:
Line 10: %define pluginspath /usr/share/ovirt-engine/ui-plugins
Line 11:
Line 12: %description
Line 19:
Line 20: %build
Line 21:
Line 22: %install
Line 23: %{__rm} -rf %{buildroot}
This %{__rm} line is not needed.
Line 24: mkdir -p %{buildroot}/%{pluginspath}
Line 25: mkdir -p %{buildroot}/%{pluginspath}/shellbox-files
Line 26: install shellbox.json %{buildroot}/%{pluginspath}
Line 27: install shellbox-files/start.html
%{buildroot}/%{pluginspath}/shellbox-files
Line 21:
Line 22: %install
Line 23: %{__rm} -rf %{buildroot}
Line 24: mkdir -p %{buildroot}/%{pluginspath}
Line 25: mkdir -p %{buildroot}/%{pluginspath}/shellbox-files
Try to use "install" instead of mkdir, and be explicit with the permissions.
For example:
install -m 755 %{buildroot}%{pluginspath}
Line 26: install shellbox.json %{buildroot}/%{pluginspath}
Line 27: install shellbox-files/start.html
%{buildroot}/%{pluginspath}/shellbox-files
Line 28:
Line 29: %clean
Line 23: %{__rm} -rf %{buildroot}
Line 24: mkdir -p %{buildroot}/%{pluginspath}
Line 25: mkdir -p %{buildroot}/%{pluginspath}/shellbox-files
Line 26: install shellbox.json %{buildroot}/%{pluginspath}
Line 27: install shellbox-files/start.html
%{buildroot}/%{pluginspath}/shellbox-files
Try to be explicit with the permissions:
install -m 644 shellbox.json ...
Line 28:
Line 29: %clean
Line 30: %{__rm} -rf %{buildroot}
Line 31:
Line 26: install shellbox.json %{buildroot}/%{pluginspath}
Line 27: install shellbox-files/start.html
%{buildroot}/%{pluginspath}/shellbox-files
Line 28:
Line 29: %clean
Line 30: %{__rm} -rf %{buildroot}
This clean is not required.
Line 31:
Line 32: %preun
Line 33: %{__rm} -rf %{pluginspath}/shellbox.json
Line 34: %{__rm} -rf %{pluginspath}/shellbox-files
Line 30: %{__rm} -rf %{buildroot}
Line 31:
Line 32: %preun
Line 33: %{__rm} -rf %{pluginspath}/shellbox.json
Line 34: %{__rm} -rf %{pluginspath}/shellbox-files
There is no need to explicitly remove the files, they are automatically removed
by RPM.
Line 35:
Line 36: %files
Line 37: %defattr(-, root, root, 0755)
Line 38: %doc README
Line 33: %{__rm} -rf %{pluginspath}/shellbox.json
Line 34: %{__rm} -rf %{pluginspath}/shellbox-files
Line 35:
Line 36: %files
Line 37: %defattr(-, root, root, 0755)
Try to avoid this %defattr, root is already the default and the permissions are
better specified when files are installed with the install command.
Line 38: %doc README
Line 39: %{pluginspath}/shellbox.json
Line 40: %{pluginspath}/shellbox-files/start.html
Line 41:
Line 39: %{pluginspath}/shellbox.json
Line 40: %{pluginspath}/shellbox-files/start.html
Line 41:
Line 42: %changelog
Line 43: * Sun Jan 20 2013 Daniel Erez <[email protected]>
This changelog line is not correctly formatted, it should be something like
this:
Sun Jan 20 2013 Daniel Erez <[email protected]> - 0.1-1
It is specially important to include the version and release numbers at the end.
--
To view, visit http://gerrit.ovirt.org/11198
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If916c7cf022e9e903cd06e55f7b8ff34851ab9c8
Gerrit-PatchSet: 1
Gerrit-Project: samples-uiplugins
Gerrit-Branch: master
Gerrit-Owner: Daniel Erez <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Einav Cohen <[email protected]>
Gerrit-Reviewer: Itamar Heim <[email protected]>
Gerrit-Reviewer: Juan Hernandez <[email protected]>
Gerrit-Reviewer: Oved Ourfali <[email protected]>
Gerrit-Reviewer: Vojtech Szocs <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
