Bug 598299 - Review Request: systemd - A System and Session Manager
Summary: Review Request: systemd - A System and Session Manager
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-06-01 03:13 UTC by Rahul Sundaram
Modified: 2010-06-14 19:33 UTC (History)
13 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-06-11 12:45:51 UTC
Type: ---
Embargoed:
adel.gadllah: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)
Patch to get the package to build on a standard F13 x86_64 system (1.34 KB, patch)
2010-06-10 14:28 UTC, David Hollis
no flags Details | Diff

Description Rahul Sundaram 2010-06-01 03:13:22 UTC
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.

Comment 1 Adel Gadllah 2010-06-01 11:55:53 UTC
(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.

Comment 2 Chen Lei 2010-06-01 12:11:54 UTC
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

Comment 3 Michal Schmidt 2010-06-01 13:01:23 UTC
(In reply to comment #1)
> 2) 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).

Comment 4 Adel Gadllah 2010-06-01 15:11:19 UTC
(In reply to comment #3)
> (In reply to comment #1)
> > 2) 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.

Comment 5 Rahul Sundaram 2010-06-02 10:14:56 UTC
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?

Comment 6 Lennart Poettering 2010-06-02 17:40:26 UTC
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

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

Comment 8 Lennart Poettering 2010-06-02 19:07:40 UTC
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.

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

Comment 10 Adel Gadllah 2010-06-02 21:04:55 UTC
(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)

Comment 11 Adel Gadllah 2010-06-02 21:07:43 UTC
(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.

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

Comment 13 Adel Gadllah 2010-06-03 17:37:29 UTC
(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

Comment 15 Richard Hughes 2010-06-10 12:55:31 UTC
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. :-)

Comment 16 David Hollis 2010-06-10 14:27:01 UTC
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

Comment 17 David Hollis 2010-06-10 14:28:15 UTC
Created attachment 422929 [details]
Patch to get the package to build on a standard F13 x86_64 system

Comment 18 Jeffrey C. Ollie 2010-06-10 14:33:03 UTC
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?

Comment 20 Adel Gadllah 2010-06-10 19:03:46 UTC
(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.fc13.src.rpm    


----
%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.

Comment 21 Rahul Sundaram 2010-06-10 19:18:55 UTC
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.fc13.src.rpm

Comment 22 Adel Gadllah 2010-06-10 20:07:42 UTC
(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.fc13.src.rpm    

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.

Comment 23 Jeffrey C. Ollie 2010-06-10 20:46:26 UTC
(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?

Comment 24 Adel Gadllah 2010-06-10 21:21:24 UTC
(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.

Comment 25 Rahul Sundaram 2010-06-11 03:26:35 UTC
Thanks everyone.  Appreciate the help

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

Comment 26 Kevin Fenzi 2010-06-11 04:51:48 UTC
CVS done (by process-cvs-requests.py).

Comment 27 Chen Lei 2010-06-11 09:28:02 UTC
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

Comment 28 Lennart Poettering 2010-06-14 19:27:12 UTC
(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!

Comment 29 Rahul Sundaram 2010-06-14 19:33:55 UTC
For those following along,  a feature proposal at

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


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