[Bug 508318] Review Request: mutter - A window manager based on metacity and clutter

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

 



Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.


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





--- Comment #15 from Peter Robinson <pbrobinson@xxxxxxxxx>  2009-07-17 17:59:52 EDT ---
SPEC: http://pbrobinson.fedorapeople.org/mutter.spec
SRPM: http://pbrobinson.fedorapeople.org/mutter-2.27.1-1.fc11.src.rpm
koji: http://koji.fedoraproject.org/koji/taskinfo?taskID=1483173

I think everything is fixed except the desktop file bit and the
SHOULD_HAVE_DEFINED section which I'll update in the morning but the rest is
now updated and it builds :-)

(In reply to comment #12)
> [OK]    *  MUST: rpmlint must be run on every package. The output should be
> posted in the review.[1]
> 
> ON SRPM:
> 
>  rpmlint /tmp/mutter-2.27.0-0.2.20090626gita13dec3.fc11.src.rpm 
> 1 packages and 0 specfiles checked; 0 errors, 0 warnings.
> 
> On RPM:
> 
> mutter.i586: W: incoherent-version-in-changelog 2.27.0-0.2
> ['2.27.0-0.2.20090626gita13dec3.fc11', '2.27.0-0.2.20090626gita13dec3']
> 
>  Doesn't matter, we'll have real tarballs soon anyways.
> 
> mutter.i586: W: shared-lib-calls-exit /usr/lib/libmutter-private.so.0.0.0
> exit@xxxxxxxxx
> 
>  None of rpmlint's damn business (libmutter-private has meta_exit, meta_fatal
> utility functions in it.)
> 
> mutter.i586: W: non-conffile-in-etc /etc/gconf/schemas/mutter.schemas
> 
>  OK.
> 
> [OK]    * MUST: The package must be named according to the Package Naming
> Guidelines .
> [OK]    * MUST: The spec file name must match the base package %{name}, in the
> format %{name}.spec unless your package has an exemption. [2] .
> [OK]    * MUST: The package must be licensed with a Fedora approved license and
> meet the Licensing Guidelines .
> [OK]    * MUST: The License field in the package spec file must match the
> actual license. [3]
> [OK]    * MUST: 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 must be included in %doc.[4]
> [OK]    * MUST: The spec file must be written in American English. [5]
> [OK]    * MUST: The spec file for the package MUST be legible. [6]
> [XX]    * MUST: The sources used to build the package must match the upstream
> source, as provided in the spec URL. Reviewers should use md5sum for this task.
> If no upstream URL can be specified for this package, please see the Source URL
> Guidelines for how to deal with this.
> 
> Source: should be
> http://download.gnome.org/sources/mutter/2.27/%{name}-%{version}.tar.bz2

Fixed.

> [XX]    * MUST: The package MUST successfully compile and build into binary
> rpms on at least one primary architecture. [7]
> 
>  Looks like the file list is out of sync with recent mutter changes; removing
> files that are no longer in Mutter and adding .po file handling, it seems to
> build OK.

Fixed.

> [NA]    * MUST: If the package does not successfully compile, build or work on
> an architecture, then those architectures should be listed in the spec in
> ExcludeArch.
> [XX]    * MUST: All build dependencies must be listed in BuildRequires, except
> for any that are listed in the exceptions section of the Packaging Guidelines ;
> inclusion of those as BuildRequires is optional. Apply common sense.
> 
> From reading the configure.in, missing BuildRequires I can find:
> 
>  gir-repository-devel
>  libXcomposite-devel
>  libSM-devel

Added. Fixed.

> [XX]    * MUST: The spec file MUST handle locales properly. This is done by
> using the %find_lang macro. Using %{_datadir}/locale/* is strictly
> forbidden.[9]
> 
> Not OK, no handling of .po files.

Added in. Fixed.

> [OK]    * MUST: Every binary RPM package (or subpackage) which stores shared
> library files (not just symlinks) in any of the dynamic linker's default paths,
> must call ldconfig in %post and %postun. [10]
> [OK]    * MUST: If the package is designed to be relocatable, the packager must
> state this fact in the request for review, along with the rationalization for
> relocation of that specific package.
> [XX]    * MUST: A package must own all directories that it creates. If it does
> not create a directory that it uses, then it should require a package which
> does create that directory. [12]
> 
> File list has: %{_libdir}/mutter/plugins/clutter/*.so and doesn't own any of
> the parent directories. (Current mutter removes the clutter/ part of this).
> Probably should just have %{_datadir}/mutter in the file list.
> 
> Needs to Requires: control-center-filesystem for
> /usr/share/gnome/wm-properties/

Fixed.

> [OK]    * MUST: A Fedora package must not list a file more than once in the
> spec file's %files listings. [13]
> [OK]    * MUST: Permissions on files must be set properly. Executables should
> be set with executable permissions, for example. Every %files section must
> include a %defattr(...) line. [14]
> [OK]    * MUST: Each package must have a %clean section
> [OK]    * MUST: Each package must consistently use macros. [16]
> [OK]    * MUST: The package must contain code, or permissable content. [17]
> [OK]    * MUST: Large documentation files must go in a -doc subpackage. (The
> definition of large is left up to the packager's best judgement, but is not
> restricted to size. Large can refer to either size or quantity). [18]
> [OK]    * MUST: If a package includes something as %doc, it must not affect the
> runtime of the application. To summarize: If it is in %doc, the program must
> run properly if it is not present. [18]
> [OK]    * MUST: Header files must be in a -devel package. [19]
> [NA]    * MUST: Static libraries must be in a -static package. [20]
> [OK]    * MUST: Packages containing pkgconfig(.pc) files must 'Requires:
> pkgconfig' (for directory ownership and usability). [21]
> [OK]    * MUST: If a package contains library files with a suffix (e.g.
> libfoo.so.1.1), then library files that end in .so (without suffix) must go in
> a -devel package. [19]
> [OK]    * MUST: In the vast majority of cases, devel packages must require the
> base package using a fully versioned dependency: Requires: %{name} =
> %{version}-%{release} [22]
> [OK]    * MUST: Packages must NOT contain any .la libtool archives, these must
> be removed in the spec if they are built.[20]
> [XX]    * MUST: Packages containing GUI applications must include a
> %{name}.desktop file, and that file must be properly installed with
> desktop-file-install in the %install section. If you feel that your packaged
> GUI application does not need a .desktop file, you must put a comment in the
> spec file with your explanation. [23]
> 
> Should be using desktop-file-install (the metacity package has
> desktop-file-install usage commented out, with a comment that the .desktop file
> is invalid, but mutter.desktop seems fine)

Not fixed yet. I will update it tomorrow.

> [OK]    * MUST: Packages must not own files or directories already owned by
> other packages. 
> [OK]    * MUST: At the beginning of %install, each package MUST run rm -rf
> %{buildroot} (or $RPM_BUILD_ROOT). [25]
> [OK]    * MUST: All filenames in rpm packages must be valid UTF-8. [26]
> 
> [NA]    *  SHOULD: If the source package does not include license text(s) as a
> separate file from upstream, the packager SHOULD query upstream to include it.
> [27]
> [NA]    * SHOULD: The description and summary sections in the package spec file
> should contain translations for supported Non-English languages, if available.
> [28]
> [NT]    * SHOULD: The reviewer should test that the package builds in mock.
> [29]
> [NT]    * SHOULD: The package should compile and build into binary rpms on all
> supported architectures. [30]
> [??]    * SHOULD: The reviewer should test that the package functions as
> described. A package should not segfault instead of running, for example.
> 
>  Got a white screen in a quick test of 'mutter --replace', but it's a slightly
> old version of Mutter, so didn't investigate further.

Will be testing this on my eeePC over the weekend.

> [OK]    * SHOULD: If scriptlets are used, those scriptlets must be sane. This
> is vague, and left up to the reviewers judgement to determine sanity. [31]
> [OK]    * SHOULD: Usually, subpackages other than devel should require the base
> package using a fully versioned dependency. [22]
> [OK]    * SHOULD: The placement of pkgconfig(.pc) files depends on their
> usecase, and this is usually for development purposes, so should be placed in a
> -devel pkg.
> [OK]    * SHOULD: If the package has file dependencies outside of /etc, /bin,
> /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the
> file instead of the file itself. [32] 
> 
> Packaging guidelines:
> 
> Other notes:
> 
>  - Should not run autogen.sh (and some of the BuildRequires: thus aren't
> needed: libotool, automake, autoconf, gnome-common0)

Was only there because of the use of a git snapshot while waiting for proper
release. Fixed.

>  - Although it makes the specfile more complex, I think the check in
> metacity.spec:
> 
> SHOULD_HAVE_DEFINED="HAVE_SM HAVE_XINERAMA HAVE_XFREE_XINERAMA HAVE_SHAPE
> HAVE_RANDR HAVE_STARTUP_NOTIFICATION"
> 
> for I in $SHOULD_HAVE_DEFINED; do
>   if ! grep -q "define $I" config.h; then
>     echo "$I was not defined in config.h"
>     grep "$I" config.h
>     exit 1
>   else
>     echo "$I was defined as it should have been"
>     grep "$I" config.h
>   fi
> done
> 
> Is probably worthwhile moving over. (And add HAVE_COMPOSITE_EXTENSION)
> 
> - From the metacity.rpm, looking at the patches:
> 
>   Patch0: default-theme.patch 
> 
>   Not needed - we share schemas with metacity
> 
>   # http://bugzilla.gnome.org/show_bug.cgi?id=558723
>   Patch4: stop-spamming-xsession-errors.patch 
>   # http://bugzilla.gnome.org/show_bug.cgi?id=135056                            
>   Patch5: dnd-keynav.patch 
>   # http://bugzilla.gnome.org/show_bug.cgi?id=584723     
>   Patch6: no-lame-dialog.patch
> 
>   All affect Mutter, but I'll take care of getting them fixed upstream, so
> let's not include them here.

OK.

>  - You should
> 
>     export GCONF_DISABLE_MAKEFILE_SCHEMA_INSTALL=1
>     make install DESTDIR=$RPM_BUILD_ROOT
>     unset GCONF_DISABLE_MAKEFILE_SCHEMA_INSTALL
> 
>    Or it will try to install the schemas into the system database on the build
> system.

Fixed.

>  - I removed dependency of this bug on gjs (not needed for Mutter by itself),
> added dependency on getting clutter built with gobject-introspection support

It can always be compiled against it at a later point.

>  - Here's my stab at a good %description
> 
>    %description
>    Mutter is a window and compositing manager that displays and manages
>    your desktop via OpenGL. Mutter combines a sophisticated display engine
>    using the Clutter toolkit with solid window-management logic inherited
>    from the Metacity window manager.
> 
>    While Mutter can be used stand-alone, it is primarily intended to be
>    used as the display core of a larger system such as gnome-shell or
>    Moblin. For this reason, Mutter is very extensible via plugins, which
>    are used both to add fancy visual efects and to rework the window
>    management behaviors to meet the needs of the environment.
> 
> A bit too much about Mutter technically and not enough about why you want to
> install Mutter, but should do. (And will most likely be sitting their unchanged
> in 10 years no matter what else has happened in the meantime....)

:-) Updated.

>  - And a %description for -devel:
> 
>     Header files and libraries for developing Mutter plugins. Also includes
>     utilities for testing Metacity/Mutter themes.

Updated.

> And finally:
> 
>  - While I was writing this, I was simultaneously getting mutter to 'make
> distcheck' succesfully. I've now uploaded a 2.27.1 tarball. It should appear at
> 
>   http://download.gnome.org/sources/mutter/2.27/
> 
>   once ftp.gnome.org finishes syncing.  

Updated.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.

_______________________________________________
Fedora-package-review mailing list
Fedora-package-review@xxxxxxxxxx
http://www.redhat.com/mailman/listinfo/fedora-package-review

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