This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours

Bug 598299

Summary: Review Request: systemd - A System and Session Manager
Product: [Fedora] Fedora Reporter: Rahul Sundaram <metherid>
Component: Package ReviewAssignee: Nobody's working on this, feel free to take it <nobody>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: adel.gadllah, andreas.bierfert, dhollis, fedora-package-review, jeff, kzak, lpoetter, mschmidt, myllynen, notting, pahan, rhughes, supercyper1
Target Milestone: ---Flags: adel.gadllah: fedora‑review+
kevin: fedora‑cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2010-06-11 08:45:51 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Attachments:
Description Flags
Patch to get the package to build on a standard F13 x86_64 system none

Description Rahul Sundaram 2010-05-31 23:13:22 EDT
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 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.
Comment 2 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
Comment 3 Michal Schmidt 2010-06-01 09:01:23 EDT
(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 11:11:19 EDT
(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 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?
Comment 6 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
Comment 7 Lennart Poettering 2010-06-02 13:43:43 EDT
Hmm, what's the plan for /cgroup/systemd? This should probably be in the package...
Comment 8 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.
Comment 9 Lennart Poettering 2010-06-02 15:22:54 EDT
"mkdir -p -m 0755 /cgroup" in %post seems sufficient to me.
Comment 10 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)
Comment 11 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.
Comment 12 Lennart Poettering 2010-06-03 13:23:18 EDT
/cgroup should definitely not be owned by systemd. It has uses outside of systemd too.
Comment 13 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
Comment 15 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. :-)
Comment 16 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
Comment 17 David Hollis 2010-06-10 10:28:15 EDT
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 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?
Comment 20 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.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 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.fc13.src.rpm
Comment 22 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.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 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?
Comment 24 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.
Comment 25 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:
Comment 26 Kevin Fenzi 2010-06-11 00:51:48 EDT
CVS done (by process-cvs-requests.py).
Comment 27 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
Comment 28 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!
Comment 29 Rahul Sundaram 2010-06-14 15:33:55 EDT
For those following along,  a feature proposal at

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