[Bug 1343710] Review Request: chrome-gnome-shell - Support for managing GNOME Shell Extensions through web browsers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=1343710



--- Comment #31 from Pete Walter <walter.pete@xxxxxxxxxx> ---
Thanks Jeremy! I've put my replies inline.


(In reply to Jeremy Newton from comment #29)
> Package Review
> ==============
> 
> Legend:
> [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
> [ ] = Manual review needed
> 
> 
> Issues:
> =======
> - gtk-update-icon-cache is invoked in %postun and %posttrans if package
>   contains icons.
>   Note: icons in chrome-gnome-shell
>   See: http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Icon_Cache
> 
> >Please add the following (explained in the wiki):
> 
> %post
> /bin/touch --no-create %{_datadir}/icons/hicolor &>/dev/null || :
> 
> %postun
> if [ $1 -eq 0 ] ; then
>     /bin/touch --no-create %{_datadir}/icons/hicolor &>/dev/null
>     /usr/bin/gtk-update-icon-cache %{_datadir}/icons/hicolor &>/dev/null || :
> fi
> 
> %posttrans
> /usr/bin/gtk-update-icon-cache %{_datadir}/icons/hicolor &>/dev/null || :

This is not needed these days. gtk3 includes a file trigger that does it
automatically.


> - Package installs a %{name}.desktop using desktop-file-install or desktop-
>   file-validate if there is such a file.
> 
> >You need to add this:
> 
> %check
> desktop-file-validate
> %{_datadir}/applications/org.gnome.ChromeGnomeShell.desktop

Done.


> ===== MUST items =====
> 
> Generic:
> [x]: Package is licensed with an open-source compatible license and meets
>      other legal requirements as defined in the legal section of Packaging
>      Guidelines.
> [x]: License field in the package spec file matches the actual license.
>      Note: Checking patched sources after %prep for licenses. Licenses
>      found: "GPL (v3 or later)", "Unknown or generated". 73 files have
>      unknown license.
> [!]: Package requires other packages for directories it uses.
>      Note: No known owner of /usr/share/icons/gnome/128x128,
>      /usr/share/icons/gnome/128x128/apps
> [!]: Package must own all directories that it creates.
>      Note: Directories without known owners:
>      /usr/share/icons/gnome/128x128/apps, /usr/share/icons/gnome,
>      /usr/share/icons/gnome/16x16/apps, /usr/share/dbus-1,
>      /usr/share/icons/gnome/128x128, /usr/share/dbus-1/services,
>      /usr/share/icons/gnome/48x48, /usr/share/icons/gnome/16x16,
>      /usr/share/icons/gnome/48x48/apps
> 
> >This is due to a missing requires, please add:
> BuildRequires:  hicolor-icon-theme
> BuildRequires:  gnome-icon-theme
> BuildRequires:  dbus
> Requires:       dbus
> Requires:       gnome-icon-theme
> Requires:       hicolor-icon-theme

Thanks. I added the Requires. The BuildRequires aren't needed here.


> [!]: Package does not own files or directories owned by other packages.
>      Note: Dirs in package are owned also by: /usr/lib64/mozilla(mozilla-
>      filesystem), /etc/opt(filesystem)
> 
> >Please remove the following line, this dir should not be owned by this package:
> %dir %{_sysconfdir}/opt

Done.


> >And change the following:
> %{_sysconfdir}/chromium/
> %{_libdir}/mozilla/
> %{_sysconfdir}/opt/chrome/
> >to:
> %{_sysconfdir}/chromium/*
> %{_libdir}/mozilla/*
> %{_sysconfdir}/opt/chrome/*

Sorry, the suggested chrome and chromium directory changes are wrong and would
result in unowned directories. Fixed the %{_libdir}/mozilla issue and added a
dep on mozilla-filesystem instead.


> [x]: %build honors applicable compiler flags or justifies otherwise.
> [x]: Package contains no bundled libraries without FPC exception.
> [x]: Changelog in prescribed format.
> [x]: Sources contain only permissible code or content.
> [-]: Development files must be in a -devel package
> [-]: Package uses nothing in %doc for runtime.
> [x]: Package consistently uses macros (instead of hard-coded directory
>      names).
> [x]: Package is named according to the Package Naming Guidelines.
> [-]: Package does not generate any conflict.
> [x]: Package obeys FHS, except libexecdir and /usr/target.
> [x]: If the package is a rename of another package, proper Obsoletes and
>      Provides are present.
> [!]: Requires correct, justified where necessary.
> 
> >See above, some requires are missing.
> 
> [x]: Spec file is legible and written in American English.
> [-]: Package contains systemd file(s) if in need.
> [-]: Useful -debuginfo package or justification otherwise.
> [-]: Package is not known to require an ExcludeArch tag.
> [x]: Package complies to the Packaging Guidelines
> [x]: Package successfully compiles and builds into binary rpms on at least
>      one supported primary architecture.
> [x]: Package installs properly.
> [x]: Rpmlint is run on all rpms the build produces.
>      Note: There are rpmlint messages (see attachment).
> [x]: If (and only if) the source package includes the text of the
>      license(s) in its own file, then that file, containing the text of the
>      license(s) for the package is included in %license.
> [x]: All build dependencies are listed in BuildRequires, except for any
>      that are listed in the exceptions section of Packaging Guidelines.
> [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
> [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
>      beginning of %install.
> [x]: Macros in Summary, %description expandable at SRPM build time.
> [x]: Package contains desktop file if it is a GUI application.
> [x]: Dist tag is present.
> [x]: Package does not contain duplicates in %files.
> [x]: Permissions on files are set properly.
> [x]: Package use %makeinstall only when make install DESTDIR=... doesn't
>      work.
> [x]: Package is named using only allowed ASCII characters.
> [x]: Package does not use a name that already exists.
> [x]: Package is not relocatable.
> [x]: Sources used to build the package match the upstream source, as
>      provided in the spec URL.
> [x]: Spec file name must match the spec package %{name}, in the format
>      %{name}.spec.
> [x]: File names are valid UTF-8.
> [x]: Large documentation must go in a -doc subpackage. Large could be size
>      (~1MB) or number of files.
>      Note: Documentation size is 0 bytes in 0 files.
> [x]: Packages must not store files under /srv, /opt or /usr/local
> 
> ===== SHOULD items =====
> 
> Generic:
> [x]: Sources can be downloaded from URI in Source: tag
> [-]: If the source package does not include license text(s) as a separate
>      file from upstream, the packager SHOULD query upstream to include it.
> [!]: Final provides and requires are sane (see attachments).
> 
> >Change the following:
> BuildRequires:  /usr/bin/base64
> BuildRequires:  /usr/bin/head
> BuildRequires:  /usr/bin/jq
> BuildRequires:  /usr/bin/sha256sum
> BuildRequires:  /usr/bin/tr
> 
> >to:
> BuildRequires:  coreutils
> BuildRequires:  jq

Can you elaborate why you want me to change this? The former is much more clear
on what is actually being used ...


> [x]: Package functions as described.
> [x]: Latest version is packaged.
> [x]: Package does not include license text files separate from upstream.
> [x]: SourceX tarball generation or download is documented.
>      Note: Package contains tarball without URL, check comments
> [-]: Description and summary sections in the package spec file contains
>      translations for supported Non-English languages, if available.
> [x]: Package should compile and build into binary rpms on all supported
>      architectures.
> [!]: %check is present and all tests pass.
> 
> >Mentioned above, missing %check
> 
> [?]: Packages should try to preserve timestamps of original installed
>      files.
> [x]: Reviewer should test that the package builds in mock.
> [x]: Buildroot is not present
> [x]: Package has no %clean section with rm -rf %{buildroot} (or
>      $RPM_BUILD_ROOT)
> [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
> [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
> [x]: SourceX is a working URL.
> [x]: Spec use %global instead of %define unless justified.
> 
> ===== EXTRA items =====
> 
> Generic:
> [x]: Rpmlint is run on all installed packages.
>      Note: There are rpmlint messages (see attachment).
> [-]: Large data in /usr/share should live in a noarch subpackage if package
>      is arched.
> [x]: Spec file according to URL is the same as in SRPM.
> 
> 
> Rpmlint
> -------
> Checking: chrome-gnome-shell-8.2-1.fc25.x86_64.rpm
>           chrome-gnome-shell-8.2-1.fc25.src.rpm
> chrome-gnome-shell.x86_64: E: no-binary
> chrome-gnome-shell.x86_64: W: no-documentation
> chrome-gnome-shell.x86_64: W: non-conffile-in-etc
> /etc/opt/chrome/policies/managed/chrome-gnome-shell.json
> chrome-gnome-shell.x86_64: W: non-conffile-in-etc
> /etc/chromium/policies/managed/chrome-gnome-shell.json
> chrome-gnome-shell.x86_64: W: non-conffile-in-etc
> /etc/chromium/native-messaging-hosts/org.gnome.chrome_gnome_shell.json
> chrome-gnome-shell.x86_64: W: non-conffile-in-etc
> /etc/opt/chrome/native-messaging-hosts/org.gnome.chrome_gnome_shell.json
> chrome-gnome-shell.x86_64: E: standard-dir-owned-by-package /etc/opt
> chrome-gnome-shell.x86_64: W: no-manual-page-for-binary chrome-gnome-shell
> 2 packages and 0 specfiles checked; 2 errors, 6 warnings.
> 
> >Does the FF plugin have to be placed in /usr/lib64/mozilla for a 64bit system? or will it work just as fine in /usr/lib/mozilla? If it needs the arched folder, you can ignore this error, if it doesn't, please change this to a noarch package.

Yes, it needs to be in /usr/lib64/mozilla.


> >Second, files placed in %{_sysconfdir} need to be prefixed with %config like so:
> 
> %config %{_sysconfdir}/opt/chrome/

No, this is incorrect. These files aren't meant to be user editable config
files.


> >The other error has been discussed above, and the remaining warnings can be ignored.
> 
> Rpmlint (installed packages)
> ----------------------------
> chrome-gnome-shell.x86_64: E: no-binary
> chrome-gnome-shell.x86_64: W: no-documentation
> chrome-gnome-shell.x86_64: W: non-conffile-in-etc
> /etc/chromium/policies/managed/chrome-gnome-shell.json
> chrome-gnome-shell.x86_64: E: standard-dir-owned-by-package /etc/opt
> chrome-gnome-shell.x86_64: W: non-conffile-in-etc
> /etc/chromium/native-messaging-hosts/org.gnome.chrome_gnome_shell.json
> chrome-gnome-shell.x86_64: W: non-conffile-in-etc
> /etc/opt/chrome/native-messaging-hosts/org.gnome.chrome_gnome_shell.json
> chrome-gnome-shell.x86_64: W: non-conffile-in-etc
> /etc/opt/chrome/policies/managed/chrome-gnome-shell.json
> chrome-gnome-shell.x86_64: W: no-manual-page-for-binary chrome-gnome-shell
> 1 packages and 0 specfiles checked; 2 errors, 6 warnings.
> 
> >Same as above
> 
> Requires
> --------
> chrome-gnome-shell (rpmlib, GLIBC filtered):
>     /usr/bin/python3
>     gnome-shell
>     python(abi)
>     python3-gobject-base
>     python3-requests
> 
> 
> 
> Provides
> --------
> chrome-gnome-shell:
>     chrome-gnome-shell
>     chrome-gnome-shell(x86-64)
>     python3.5dist(chrome-gnome-shell)
>     python3dist(chrome-gnome-shell)
> 
> 
> 
> Generated by fedora-review 0.6.1 (f03e4e7) last change: 2016-05-02
> Command line :/usr/bin/fedora-review -b 1343710
> Buildroot used: fedora-25-x86_64
> Active plugins: Generic, Shell-api
> Disabled plugins: Java, C/C++, Python, fonts, SugarActivity, Ocaml, Perl,
> Haskell, R, PHP
> Disabled flags: EXARCH, DISTTAG, EPEL5, BATCH, EPEL6

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
_______________________________________________
package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx




[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]