[Bug 598299] New: Review Request: systemd - A System and Session Manager

[Red Hat Bugzilla]

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

Summary: Review Request: systemd - A System and Session Manager

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

Summary: Review Request: systemd - A System and Session Manager
           Product: Fedora
           Version: rawhide
          Platform: All
        OS/Version: Linux
            Status: NEW
          Severity: medium
          Priority: medium
         Component: Package Review
        AssignedTo: [e-mail]
        ReportedBy: [e-mail]
         QAContact: [e-mail]
                CC: [e-mail], [e-mail]
   Estimated Hours: 0.0
    Classification: Fedora

Spec URL: http://sundaram.fedorapeople.org/packages/systemd.spec
SRPM URL:
http://sundaram.fedorapeople.org/packages/systemd-0-0.0.20100602git.src.rpm

Description: 
Systemd is a system and session manager compatible with SysV init and LSB init 
script headers. Systemd has aggressive parallelization capabilities, uses D-Bus 
activation for starting services and keeps track of processes using cgroups.

--

This will only build on rawhide due to the build requirements, in particular
udev needs to be a higher version than what is in F-13. I am filing this review
request on behalf of Lennart since he isn't keen on packaging.  I have
confirmed  that it is ok by him.  I will add him as the primary maintainer once
the review process is over.

[Bug 598299] Review Request: systemd - A System and Session Manager

[Red Hat Bugzilla]

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=598299

Adel Gadllah  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]
               Flag|                            |fedora-review?

--- Comment #1 from Adel Gadllah  2010-06-01 07:55:53 EDT ---
(In reply to comment #0)
> Spec URL: http://sundaram.fedorapeople.org/packages/systemd.spec
> SRPM URL:
> http://sundaram.fedorapeople.org/packages/systemd-0-0.0.20100602git.src.rpm
>
> Description: 
> Systemd is a system and session manager compatible with SysV init and LSB init 
> script headers. Systemd has aggressive parallelization capabilities, uses D-Bus 
> activation for starting services and keeps track of processes using cgroups.
>
> --
>
> This will only build on rawhide due to the build requirements, in particular
> udev needs to be a higher version than what is in F-13. I am filing this review
> request on behalf of Lennart since he isn't keen on packaging.  I have
> confirmed  that it is ok by him.  I will add him as the primary maintainer once
> the review process is over.

OK here are some initial comments:

1) Does not build in rawhide:
http://koji.fedoraproject.org/koji/taskinfo?taskID=2221974
2) No %clean
3) Missing instructions on how the tarball was generated
4) Please add an abbreviated git commit id to the release (date is not unique)
5) "%{_mandir}/man?/*.[0-9]* " no need for using fancy regex here
6) rpmlint output:
------
systemd.src: W: spelling-error %description -l en_US init -> unit, int, nit
systemd.src: W: spelling-error %description -l en_US parallelization ->
parallelism, parallelogram, channelization
systemd.src: W: spelling-error %description -l en_US cgroups -> groups, c
groups, Citigroup
systemd.src:16: W: macro-in-comment %{name}
systemd.src:16: W: macro-in-comment %{version}
systemd.src:49: E: hardcoded-library-path in /lib/systemd/
systemd.src: W: no-cleaning-of-buildroot %install
systemd.src: W: no-cleaning-of-buildroot %clean
systemd.src: W: no-buildroot-tag
systemd.src: W: no-%clean-section
systemd.src: W: invalid-url Source0: systemd-2010-06-02.tar.xz
-------

Can be mostly ignored.

No full review possible due to build failure.
I will do a proper review once you fix the noted issues and the build.

[Bug 598299] Review Request: systemd - A System and Session Manager

[Red Hat Bugzilla]

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=598299

--- Comment #2 from Chen Lei  2010-06-01 08:11:54 EDT ---
Is there any reason to use --libexecdir=/lib/systemd  instead of
{_prefix}/libexecdir in this package?

libexecdir=/lib/%{name} looks like the default libexec path in debian/ubuntu.

http://fedoraproject.org/wiki/PackagingGuidelines#Libexecdir

[Bug 598299] Review Request: systemd - A System and Session Manager

[Red Hat Bugzilla]

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=598299

Michal Schmidt  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #3 from Michal Schmidt  2010-06-01 09:01:23 EDT ---
(In reply to comment #1)
> No %clean

Writing an explicit %clean section is not necessary anymore since RPM 4.8
because a sane default one is now added (http://rpm.org/ticket/81).

[Bug 598299] Review Request: systemd - A System and Session Manager

[Red Hat Bugzilla]

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=598299

--- Comment #4 from Adel Gadllah  2010-06-01 11:11:19 EDT ---
(In reply to comment #3)
> (In reply to comment #1)
> > No %clean
>
> Writing an explicit %clean section is not necessary anymore since RPM 4.8
> because a sane default one is now added (http://rpm.org/ticket/81).

Ah, OK and as it is a rawhide only package, it should be fine ... so scratch
that from the list.

[Bug 598299] Review Request: systemd - A System and Session Manager

[Red Hat Bugzilla]

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=598299

--- Comment #5 from Rahul Sundaram  2010-06-02 06:14:56 EDT ---
The man pages are being built from docbook xml files on the fly. I tried adding
libxslt as a BR but it still fails trying to grab xsl file from the net.  Seems
I am still missing some other dependency but not sure which one. Suggestions?

[Bug 598299] Review Request: systemd - A System and Session Manager

[Red Hat Bugzilla]

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=598299

--- Comment #6 from Lennart Poettering  2010-06-02 13:40:26 EDT ---
You need a dep on "docbook-style-xsl" for the docbook stuff.

I have now added --with-rootdir= to deal with splitting up the appropriate
stuff between / and /usr. Just pass it to configure with an empty argument to
get things right.

Might be good to use the "official" package description we now have upstream:

http://lists.freedesktop.org/archives/systemd-devel/2010-June/000055.html

[Bug 598299] Review Request: systemd - A System and Session Manager

[Red Hat Bugzilla]

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=598299

--- Comment #7 from Lennart Poettering  2010-06-02 13:43:43 EDT ---
Hmm, what's the plan for /cgroup/systemd? This should probably be in the
package...

[Bug 598299] Review Request: systemd - A System and Session Manager

[Red Hat Bugzilla]

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=598299

--- Comment #8 from Lennart Poettering  2010-06-02 15:07:40 EDT ---
git master now mounts a tmpfs to /cgroup. That means that the /cgroup/systemd
dir can be dynamically created at runtime by systemd. However, the /cgroup dir
must exist on boot, since / tends to be ro during early boot. That means
something or somebody must create /cgroup on package installation. I'd probably
just do an mkdir in %post, if that's acceptable.

[Bug 598299] Review Request: systemd - A System and Session Manager

[Red Hat Bugzilla]

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=598299

--- Comment #9 from Lennart Poettering  2010-06-02 15:22:54 EDT ---
"mkdir -p -m 0755 /cgroup" in %post seems sufficient to me.

[Bug 598299] Review Request: systemd - A System and Session Manager

[Red Hat Bugzilla]

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=598299

--- Comment #10 from Adel Gadllah  2010-06-02 17:04:55 EDT ---
(In reply to comment #9)
> "mkdir -p -m 0755 /cgroup" in %post seems sufficient to me.

Why can't you do this in your install target? (i.e make install)

[Bug 598299] Review Request: systemd - A System and Session Manager

[Red Hat Bugzilla]

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=598299

--- Comment #11 from Adel Gadllah  2010-06-02 17:07:43 EDT ---
(In reply to comment #10)
> (In reply to comment #9)
> > "mkdir -p -m 0755 /cgroup" in %post seems sufficient to me.
>
> Why can't you do this in your install target? (i.e make install)

Or atleast in %install ... don't see a reason why this has to be done in %post
unless you don't systemd to own it.

In that case adding it to the filesystem package would be sufficient.

[Bug 598299] Review Request: systemd - A System and Session Manager

[Red Hat Bugzilla]

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=598299

--- Comment #12 from Lennart Poettering  2010-06-03 13:23:18 EDT ---
/cgroup should definitely not be owned by systemd. It has uses outside of
systemd too.

[Bug 598299] Review Request: systemd - A System and Session Manager

[Red Hat Bugzilla]

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=598299

--- Comment #13 from Adel Gadllah  2010-06-03 13:37:29 EDT ---
(In reply to comment #12)
> /cgroup should definitely not be owned by systemd. It has uses outside of
> systemd too.

OK, that makes sense, filed a bug against filesystem to get it added there:
https://bugzilla.redhat.com/show_bug.cgi?id=599664

[Bug 598299] Review Request: systemd - A System and Session Manager

[Red Hat Bugzilla]

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=598299

--- Comment #14 from Rahul Sundaram  2010-06-09 16:21:40 EDT ---

http://sundaram.fedorapeople.org/packages/systemd.spec
http://sundaram.fedorapeople.org/packages/systemd-0-0.1.20090609git2f198e.fc...

scratch build
http://koji.fedoraproject.org/koji/taskinfo?taskID=2241423

[Bug 598299] Review Request: systemd - A System and Session Manager

[Red Hat Bugzilla]

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=598299

Richard Hughes  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #15 from Richard Hughes  2010-06-10 08:55:31 EDT ---
Rahul, by putting "--with-rootdir=/usr" the systemd binary gets put in /usr/bin
rather than /bin -- I'm pretty sure something as low level as an init system
deserves to be in /bin. :-)

[Bug 598299] Review Request: systemd - A System and Session Manager

[Red Hat Bugzilla]

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=598299

David Hollis  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #16 from David Hollis  2010-06-10 10:27:01 EDT ---
I encountered a few errors while trying to build this:
1) It's not 2009 any more (git_date)
2) The x86_64 lib workaround didn't seem to fly, I workaround it by
(re)defining _libdir to /lib unconditionally
3) /etc/init.d didn't exist so it bombed in %install
4) Redefining _bindir to /bin and clearing the --with-rootdir= part managed to
get the binaries in the right place

I'll attach a diff of the spec that I got working

[Bug 598299] Review Request: systemd - A System and Session Manager

[Red Hat Bugzilla]

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=598299

--- Comment #17 from David Hollis  2010-06-10 10:28:15 EDT ---
Created an attachment (id=422929)
 --> (https://bugzilla.redhat.com/attachment.cgi?id=422929)
Patch to get the package to build on a standard F13 x86_64 system

[Bug 598299] Review Request: systemd - A System and Session Manager

[Red Hat Bugzilla]

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=598299

Jeffrey C. Ollie  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #18 from Jeffrey C. Ollie  2010-06-10 10:33:03 EDT ---
I just tried the scratch build mentioned in comment 14 by installing the
systemd RPM on my rawhide box and then naïvely rebooting and adding
"init=/usr/bin/systemd" to the kernel command line.  Right after mounting the
cgroup file systems it looked like systemd segfaulted and then I got a kernel
panic.  Is there some other magic that I need to do to try out systemd or am I
encountering an actual bug.  What's the best way to debug this problem other
than posting screenshots of the kernel panic?

[Bug 598299] Review Request: systemd - A System and Session Manager

[Red Hat Bugzilla]

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=598299

--- Comment #19 from Rahul Sundaram  2010-06-10 13:39:25 EDT ---
Thanks David Hollis

http://sundaram.fedorapeople.org/packages/systemd.spec
http://sundaram.fedorapeople.org/packages/systemd-0-0.1.20100610git2f198e.fc...

[Bug 598299] Review Request: systemd - A System and Session Manager

[Red Hat Bugzilla]

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=598299

--- Comment #20 from Adel Gadllah  2010-06-10 15:03:46 EDT ---
(In reply to comment #18)
> I just tried the scratch build mentioned in comment 14 by installing the
> systemd RPM on my rawhide box and then naïvely rebooting and adding
> "init=/usr/bin/systemd" to the kernel command line.  Right after mounting the
> cgroup file systems it looked like systemd segfaulted and then I got a kernel
> panic.  Is there some other magic that I need to do to try out systemd or am I
> encountering an actual bug.  What's the best way to debug this problem other
> than posting screenshots of the kernel panic?

According to Lennart a recent commit "broke everything" ... so it is a known
issue that will hopefully resolved soon.

(In reply to comment #19)
> Thanks David Hollis
>
> http://sundaram.fedorapeople.org/packages/systemd.spec
> http://sundaram.fedorapeople.org/packages/systemd-0-0.1.20100610git2f198e.fc...

----
%post
mkdir -p -m 0755 /cgroup
----

This isn't needed as the directory is created and owned by libcgroup.

-----
%global git_date    20100610
%global git_version 2f198e
%define _bindir  /bin
%define _libdir  /lib
-----

Use %global for the later two too.

rpmlint:

------
systemd.src:21: W: macro-in-comment %{name}
systemd.src:21: W: macro-in-comment %{git_version}
systemd.src:21: W: macro-in-comment %{version}
systemd.src:21: W: macro-in-comment %{git_date}
systemd.src:21: W: macro-in-comment %{git_version}
systemd.src:24: W: macro-in-comment %{name}
systemd.src:24: W: macro-in-comment %{version}
-------

Even though none of those will cause any issues; avoid any macros in comments
(multiline macros can cause all sort of weirdness).

---
systemd.src:6: W: mixed-use-of-spaces-and-tabs (spaces: line 6, tab: line 3)
---

Either stick to spaces or tabs but don't use both.

Other than that it looks good; it builds fine there is still still some other
noise from rpmlint but it can be ignored.

Please fix the issues noted above; once you done that there shouldn't be much
blocking approval.

[Bug 598299] Review Request: systemd - A System and Session Manager

[Red Hat Bugzilla]

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=598299

--- Comment #21 from Rahul Sundaram  2010-06-10 15:18:55 EDT ---
I haven't removed the macros from comments since I don't want to be hardcoding
the values and adjusting them every bump. Just silenced it.  Should be a
temporary thing till the point we switch over to releases.

http://sundaram.fedorapeople.org/packages/systemd.spec
http://sundaram.fedorapeople.org/packages/systemd-0-0.3.20100610git2f198e.fc...

[Bug 598299] Review Request: systemd - A System and Session Manager

[Red Hat Bugzilla]

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=598299

Adel Gadllah  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|fedora-review?              |fedora-review+

--- Comment #22 from Adel Gadllah  2010-06-10 16:07:42 EDT ---
(In reply to comment #21)
> I haven't removed the macros from comments since I don't want to be hardcoding
> the values and adjusting them every bump. Just silenced it.  Should be a
> temporary thing till the point we switch over to releases.

Yeah got that; another option would be to just escape them (%% instead of %).
> http://sundaram.fedorapeople.org/packages/systemd.spec
> http://sundaram.fedorapeople.org/packages/systemd-0-0.3.20100610git2f198e.fc...

Looks good, I have not actually tried to run it as it is known broken but the
spec is clean now and it builds fine.

=> Approved.

[Bug 598299] Review Request: systemd - A System and Session Manager

[Red Hat Bugzilla]

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=598299

--- Comment #23 from Jeffrey C. Ollie  2010-06-10 16:46:26 EDT ---
(In reply to comment #20)
> (In reply to comment #18)
> > Right after mounting the
> > cgroup file systems it looked like systemd segfaulted and then I got a kernel
> > panic.
>
> According to Lennart a recent commit "broke everything" ... so it is a known
> issue that will hopefully resolved soon.

I looked on the systemd-devel list and didn't see any mention...  Is there a
specific commit that can be reverted easily or is there a deeper problem?

[Bug 598299] Review Request: systemd - A System and Session Manager

[Red Hat Bugzilla]

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=598299

--- Comment #24 from Adel Gadllah  2010-06-10 17:21:24 EDT ---
(In reply to comment #23)
> (In reply to comment #20)
> > (In reply to comment #18)
> > > Right after mounting the
> > > cgroup file systems it looked like systemd segfaulted and then I got a kernel
> > > panic.
> >
> > According to Lennart a recent commit "broke everything" ... so it is a known
> > issue that will hopefully resolved soon.
>
> I looked on the systemd-devel list and didn't see any mention...  Is there a
> specific commit that can be reverted easily or is there a deeper problem?

He said than on IRC ... no idea have not actually looked at it.

I'd expect it to be fixed soon though.

[Bug 598299] Review Request: systemd - A System and Session Manager

[Red Hat Bugzilla]

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=598299

Rahul Sundaram  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
               Flag|                            |fedora-cvs?

--- Comment #25 from Rahul Sundaram  2010-06-10 23:26:35 EDT ---
Thanks everyone.  Appreciate the help

New Package CVS Request
=======================
Package Name: systemd 
Short Description: A System and Session Manager
Owners: lennart sundaram
Branches: 
InitialCC:

[Bug 598299] Review Request: systemd - A System and Session Manager

[Red Hat Bugzilla]

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=598299

--- Comment #26 from Kevin Fenzi  2010-06-11 00:51:48 EDT ---
CVS done (by process-cvs-requests.py).

[Bug 598299] Review Request: systemd - A System and Session Manager

[Red Hat Bugzilla]

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=598299

--- Comment #27 from Chen Lei  2010-06-11 05:28:02 EDT ---
Some trival issues that can be fixed after the feature is approved(thus upstart
should be obsoleted)

1.This package install 64bit elf files to /lib.
/lib/systemd/systemd-cgroups-agent
/lib/systemd/systemd-initctl
/lib/systemd/systemd-logger

2.I suggest not to override system rpm macros in spec, using /bin,/lib or
/%{_lib} in %file directly to keep consistency with others core packages(e.g.
glibc iptables) will be better.

3.%configure --sbindir=/sbin  --libexecdir=%{_prefix}/libexec --with-rootdir= 
--with-distro=fedora CFLAGS="%{optflags}"

--libexecdir=%{_prefix}/libexec and CFLAGS="%{optflags}"  seems not needed.

See rpm --eval %configure

[Bug 598299] Review Request: systemd - A System and Session Manager

[Red Hat Bugzilla]

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=598299

Rahul Sundaram  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |CLOSED
         Resolution|                            |RAWHIDE

[Bug 598299] Review Request: systemd - A System and Session Manager

[Red Hat Bugzilla]

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=598299

--- Comment #28 from Lennart Poettering  2010-06-14 15:27:12 EDT ---
(In reply to comment #27)
> Some trival issues that can be fixed after the feature is approved(thus upstart
> should be obsoleted)
>
> 1.This package install 64bit elf files to /lib.
> /lib/systemd/systemd-cgroups-agent
> /lib/systemd/systemd-initctl
> /lib/systemd/systemd-logger

This is intended that way. It's like udev's utility binaries in /lib/udev/

Rahul, thanks a lot for doing this work. Very much appreciated!

[Bug 598299] Review Request: systemd - A System and Session Manager

[Red Hat Bugzilla]

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=598299

--- Comment #29 from Rahul Sundaram  2010-06-14 15:33:55 EDT ---
For those following along,  a feature proposal at

https://fedoraproject.org/wiki/Features/systemd