Bug 984700 - (sddm) Review Request: sddm - QML based X11 desktop manager
Review Request: sddm - QML based X11 desktop manager
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Christopher Meng
Fedora Extras Quality Assurance
:
Depends On:
Blocks: qt-reviews
  Show dependency treegraph
 
Reported: 2013-07-15 13:49 EDT by Martin Bříza
Modified: 2016-10-05 07:12 EDT (History)
9 users (show)

See Also:
Fixed In Version: sddm-0.1.0-5.fc18
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-08-05 20:20:47 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
i: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Martin Bříza 2013-07-15 13:49:01 EDT
Spec URL: http://mbriza.fedorapeople.org/sddm/sddm.spec
SRPM URL: http://mbriza.fedorapeople.org/sddm/sddm-0.1.0-1.src.rpm
Description: SDDM is a modern display manager for X11 aiming to be fast, simple and beautiful. It uses modern technologies like QtQuick, which in turn gives the designer the ability to create smooth, animated user interfaces.
Fedora Account System Username: mbriza
Comment 1 Christopher Meng 2013-07-15 20:14:47 EDT
Initial review:

1. "BuildRequires:  gcc-c++" is not needed, you can remove it.

2. You can simplify "%setup -q -n sddm-%{version}" to "%setup -q"

3. Systemd units files are handled incorrectly IMO.

Ref: http://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd

4. Release tag should be 1%{?dist}, after the fix should be 2%{?dist}
Comment 2 Martin Bříza 2013-07-16 06:51:18 EDT
Spec URL: http://mbriza.fedorapeople.org/sddm/sddm.spec
SRPM URL: http://mbriza.fedorapeople.org/sddm/sddm-0.1.0-2.fc19.src.rpm
Thanks for the review, Christopher, I fixed the following problems:
- Removed unneeded BuildRequires.
- Fixed systemd scriptlets.
- Fixed release.
- Simplified setup.
- Added Requires needed for basic function.
- Added Provides for graphical login.
Comment 3 Christopher Meng 2013-07-17 01:17:57 EDT
Hmmm...

Why do we need "BuildRequires:  systemd-devel"? Isn't it "BuildRequires:  systemd"?
Comment 4 Kevin Kofler 2013-07-17 19:05:02 EDT
Not if this links to one of the C libraries that are part of systemd.
Comment 5 Christopher Meng 2013-07-17 20:02:54 EDT
Hi,

1. This morning I tried a mock build but failed with:

Executing(%build): /bin/sh -e /var/tmp/rpm-tmp.e633jH
+ umask 022
+ cd /builddir/build/BUILD
+ cd sddm-0.1.0
+ mkdir -p i686-redhat-linux-gnu
+ sed -i s/-march=native// CMakeLists.txt
+ pushd i686-redhat-linux-gnu
~/build/BUILD/sddm-0.1.0/i686-redhat-linux-gnu ~/build/BUILD/sddm-0.1.0
+ '%{cmake}' ..
/var/tmp/rpm-tmp.e633jH: line 35: fg: no job control
error: Bad exit status from /var/tmp/rpm-tmp.e633jH (%build)
    Bad exit status from /var/tmp/rpm-tmp.e633jH (%build)
RPM build errors:
Child return code was: 1
EXCEPTION: Command failed. See logs for output.
 # ['bash', '--login', '-c', 'rpmbuild -bb --target i686 --nodeps builddir/build/SPECS/sddm.spec']
Traceback (most recent call last):
  File "/usr/lib/python2.7/site-packages/mockbuild/trace_decorator.py", line 70, in trace
    result = func(*args, **kw)
  File "/usr/lib/python2.7/site-packages/mockbuild/util.py", line 359, in do
    raise mockbuild.exception.Error, ("Command failed. See logs for output.\n # %s" % (command,), child.returncode)
Error: Command failed. See logs for output.
 # ['bash', '--login', '-c', 'rpmbuild -bb --target i686 --nodeps builddir/build/SPECS/sddm.spec']
LEAVE do --> EXCEPTION RAISED

Something is wrong in the cmake, I see two periods.

2. And one more question about your spec, I can see these lines in %files:

%config   %{_sysconfdir}/pam.d/sddm
%config   %{_sysconfdir}/sddm.conf

Can these be %config(noreplace)?

3. I think you should choose tarball https://github.com/sddm/sddm/archive/0.1.0.tar.gz

as Source0 as this software will grow bigger and bigger.
Comment 6 Kevin Kofler 2013-07-17 20:17:19 EDT
> + '%{cmake}' ..
> /var/tmp/rpm-tmp.e633jH: line 35: fg: no job control

Missing "BuildRequires: cmake".

("fg: no job control" is usually the result of an undefined RPM macro. RPM then leaves the macro unexpanded, so bash tries to interpret it as some job control thingy, which obviously fails.)

> Something is wrong in the cmake, I see two periods.

No, that is perfectly fine cmake command-line syntax. (CMake takes a directory on the command line, .. is the parent directory, which is what you want if you're building in a subdirectory.)

> 3. I think you should choose tarball
> https://github.com/sddm/sddm/archive/0.1.0.tar.gz
>
> as Source0 as this software will grow bigger and bigger.

tar.gz or zip doesn't make much difference compression-wise, they're both using deflate as the algorithm. The tar.gz might be slightly smaller because it is always a solid archive (it first tars up everything and then compresses the whole thing as one file), but I don't expect a big difference. A tar.xz or at least tar.bz2 would be helpful size-wise, if github can deliver that (but I wouldn't recompress the upstream tarball if they only ship tar.gz, doing that is frowned upon because it makes it harder to verify authenticity).

That said, I'd pick the tar.gz over the zip for other practical reasons, e.g. better support for unixy stuff such as symlinks, special permissions etc.
Comment 7 Martin Bříza 2013-07-18 03:49:56 EDT
Spec URL: http://mbriza.fedorapeople.org/sddm/sddm.spec
SRPM URL: http://mbriza.fedorapeople.org/sddm/sddm-0.1.0-2.fc19.src.rpm

(In reply to Christopher Meng from comment #3)
> Why do we need "BuildRequires:  systemd-devel"? Isn't it "BuildRequires: 
> systemd"?

You're right, I thought the systemd pkg-config module is in the -devel package. Fixed.

(In reply to Christopher Meng from comment #5)
> Something is wrong in the cmake, I see two periods.

Missing BuildRequires: cmake; fixed.

> 2. And one more question about your spec, I can see these lines in %files:
> 
> %config   %{_sysconfdir}/pam.d/sddm
> %config   %{_sysconfdir}/sddm.conf
> 
> Can these be %config(noreplace)?

Fixed.

> 3. I think you should choose tarball
> https://github.com/sddm/sddm/archive/0.1.0.tar.gz
> 
> as Source0 as this software will grow bigger and bigger.

Changed as per Kevin's reasoning in comment #6. Unfortunately, there isn't an option to download xz or bz2 packages.
Comment 8 Martin Bříza 2013-07-18 05:33:57 EDT
SRPM URL: http://mbriza.fedorapeople.org/sddm/sddm-0.1.0-3.fc19.src.rpm
...
Comment 9 Christopher Meng 2013-07-19 00:57:57 EDT
No problems here.

Only 1 thing, you should add README and CONTRIBUTORS and especially COPYING as %doc.

Once you upload the -4 spec, I'll approve.
Comment 10 Martin Bříza 2013-07-22 06:01:39 EDT
Spec URL: http://mbriza.fedorapeople.org/sddm/sddm.spec
SRPM URL: http://mbriza.fedorapeople.org/sddm/sddm-0.1.0-4.fc19.src.rpm

(In reply to Christopher Meng from comment #9)
> Only 1 thing, you should add README and CONTRIBUTORS and especially COPYING
> as %doc.

Done.

Thank you and sorry for the delay.
Comment 11 Christopher Meng 2013-07-22 06:51:18 EDT
Alright, approved.
Comment 12 Neal Becker 2013-07-22 07:18:01 EDT
I'd like to test sddm on f19.  I have built/installed 0.1.0-4.  I did not see any instructions on how to test - hints?
Comment 13 Martin Bříza 2013-07-22 07:24:48 EDT
Neal, 
using the following command (as root):
 systemctl enable --force sddm.service
will replace your current DM with SDDM. On the next reboot, you'll get SDDM.
If you won't want to reboot, switch to an other VT ([Ctrl + Alt + F6] for example), log in as root and use the following commands:
 systemctl stop display-manager.service # this will switch you to VT1, most probably, so then switch to VT6 again with [Alt + F6]
 systemctl start sddm.service
I have described some basic steps on how to test the package on https://fedoraproject.org/wiki/Changes/SDDMinsteadOfKDM . However, many of the steps don't apply, as some needed features aren't implemented yet in this version.
Comment 14 Neal Becker 2013-07-22 08:19:01 EDT
Well that failed.
2 notes on my config:
* I'm using kde
* I'm using nvidia blob

Jul 22 08:13:18 nbecker1 sddm[27151]: DAEMON: Initializing...
Jul 22 08:13:18 nbecker1 sddm[27151]: DAEMON: Adding new display ":0" ...
Jul 22 08:13:18 nbecker1 sddm[27151]: DAEMON: Starting...
Jul 22 08:13:18 nbecker1 sddm[27151]: DAEMON: Generating cookie...
Jul 22 08:13:18 nbecker1 sddm[27151]: DAEMON: Adding cookie to "/var/run/xauth/A:0-CiEIvq"
Jul 22 08:13:18 nbecker1 sddm[27151]: /usr/bin/xauth:  file /var/run/xauth/A:0-CiEIvq does not exist
Jul 22 08:13:18 nbecker1 sddm[27151]: DAEMON: Display server starting...
Jul 22 08:13:18 nbecker1 sddm[27151]: DAEMON: Display server started.
Jul 22 08:13:18 nbecker1 sddm[27151]: DAEMON: Socket server starting...
Jul 22 08:13:18 nbecker1 sddm[27151]: DAEMON: Socket server started.
Jul 22 08:13:18 nbecker1 sddm[27151]: DAEMON: Greeter starting...
Jul 22 08:13:18 nbecker1 sddm[27151]: DAEMON: Greeter started.
Jul 22 08:13:20 nbecker1 sddm[27151]: DAEMON: Message received from greeter: Connect
Jul 22 08:13:26 nbecker1 sddm[27151]: DAEMON: Message received from greeter: Login
Jul 22 08:13:26 nbecker1 systemd-logind[397]: New session 52 of user nbecker.
Jul 22 08:13:26 nbecker1 sddm[27151]: DAEMON: User session started.
Jul 22 08:13:28 nbecker1 fprintd[26972]: ** Message: No devices in use, exit
Jul 22 08:13:29 nbecker1 sddm[27151]: DAEMON: User session ended.
Jul 22 08:13:29 nbecker1 sddm[27151]: DAEMON: Greeter stopping...
Jul 22 08:13:29 nbecker1 sddm[27151]: DAEMON: Greeter stopped.
Jul 22 08:13:29 nbecker1 sddm[27151]: DAEMON: Socket server stopping...
Jul 22 08:13:29 nbecker1 sddm[27151]: DAEMON: Socket server stopped.
Jul 22 08:13:29 nbecker1 sddm[27151]: DAEMON: Display server stopping...
Jul 22 08:13:31 nbecker1 systemd[1]: Started Getty on tty6.
Jul 22 08:13:31 nbecker1 abrt[27346]: Saved core dump of pid 27154 (/usr/bin/Xorg) to /var/tmp/abrt/ccpp-2013-07-22-08:13:31-27154 (23814144 bytes)
Jul 22 08:13:31 nbecker1 abrtd: Directory 'ccpp-2013-07-22-08:13:31-27154' creation detected
Jul 22 08:13:31 nbecker1 abrtd: Generating core_backtrace
Jul 22 08:13:31 nbecker1 abrtd: Generating backtrace
Jul 22 08:13:31 nbecker1 abrtd: Backtrace is generated, 8355 bytes
Jul 22 08:13:31 nbecker1 abrtd: Cannot disassemble function at 0x0, not computing fingerprint
Jul 22 08:13:31 nbecker1 abrtd: dwarf_next_cfi failed for /usr/lib64/nvidia-304xx/libnvidia-glcore.so.304.88: invalid DWARF
Jul 22 08:13:31 nbecker1 abrtd: Failed to read .eh_frame function ranges from /usr/lib64/nvidia-304xx/libnvidia-glcore.so.304.88
Jul 22 08:13:31 nbecker1 abrtd: Core backtrace is generated and saved, 1337 bytes
Comment 15 Rex Dieter 2013-07-22 08:37:22 EDT
Per

Jul 22 08:13:18 nbecker1 sddm[27151]: DAEMON: Adding cookie to
"/var/run/xauth/A:0-CiEIvq"
Jul 22 08:13:18 nbecker1 sddm[27151]: /usr/bin/xauth:  file
/var/run/xauth/A:0-CiEIvq does not exist

Looks like you'll want to create/own this dir at least, though it may be wise to configure the defaults to use something sddm-specific, like:
/var/run/sddm/
instead.

Anyway, see lightdm for an example of how to do this,
http://pkgs.fedoraproject.org/cgit/lightdm.git/tree/lightdm.spec
http://pkgs.fedoraproject.org/cgit/lightdm.git/tree/lightdm-tmpfiles.conf
Comment 16 Martin Bříza 2013-07-25 08:35:02 EDT
New Package SCM Request
=======================
Package Name: sddm
Short Description: QML based X11 desktop manager
Owners: mbriza rdieter kkofler ltinkl dvratil jgrulich
Branches: f18 f19
InitialCC:
Comment 17 Martin Bříza 2013-07-25 08:37:32 EDT
Rex, thank you, I'll address it when I push the package into the repository.
Comment 18 Gwyn Ciesla 2013-07-25 10:37:30 EDT
Git done (by process-git-requests).
Comment 19 Martin Bříza 2013-07-25 11:21:48 EDT
Back to the xauth problem - The directory is being created on runtime, so I'll just change the path to /var/run/sddm to not complicate things for now.
Message about the nonexistent file is benign.
I'm just pushing the sources and will build the package.

Neal could you please report a bug against sddm with the backtrace? We'll discuss the matter further on in there.
Thank you.
Comment 20 Fedora Update System 2013-07-25 11:33:57 EDT
sddm-0.1.0-5.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/sddm-0.1.0-5.fc19
Comment 21 Fedora Update System 2013-07-25 11:34:56 EDT
sddm-0.1.0-5.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/sddm-0.1.0-5.fc18
Comment 22 Fedora Update System 2013-07-25 20:21:34 EDT
sddm-0.1.0-5.fc19 has been pushed to the Fedora 19 testing repository.
Comment 23 Fedora Update System 2013-08-05 20:20:47 EDT
sddm-0.1.0-5.fc19 has been pushed to the Fedora 19 stable repository.
Comment 24 Fedora Update System 2013-08-05 20:21:41 EDT
sddm-0.1.0-5.fc18 has been pushed to the Fedora 18 stable repository.
Comment 25 Raphael Groner (DAASI International) 2016-10-05 05:04:14 EDT
Can someone please remove the alias? It's not possible to generally search for sddm bugs. I am not allowed to edit this bug.
Comment 26 Igor Gnatenko 2016-10-05 05:28:26 EDT
(In reply to Raphael Groner (DAASI International) from comment #25)
> Can someone please remove the alias? It's not possible to generally search
> for sddm bugs. I am not allowed to edit this bug.

please report bug to bugzilla administrators.
Comment 27 Rex Dieter 2016-10-05 07:12:24 EDT
I would prefer the alias remain

Note You need to log in before you can comment on or make changes to this bug.