[Bug 717748] Review Request: LTTng-UST - LTTng Userspace Tracer

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

 



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

--- Comment #12 from Scott Tsai <scottt.tw@xxxxxxxxx> ---
(In reply to comment #11)
Yannick,
After your libtoolize change I can now build the packages successfully.
I suspect you didn't see the failture from comment #9 before because you were
using rpmbuild locally and you already have a copy of the lttng-ust libraries
in /usr/lib6{,64}. (Having the libraries in the default linker search path is
equivalent to helping ld find them via -rpath-link.)

Comments on the .spec file follows:

1. License should be "LGPLv2 and GPLv2"

liblttng-ust-ctl/ustctl.c is GPLv2 only and is used to build
liblttng-ust-ctl.so. ust-ctl.h is also GPLv2 only and shipped in the -devel
package.

The assertion in the COPYING file that ustctl.c is only used by lttng-sessiond
is not true when we ship it in a library with a public header.

BTW, lttng-ust being LGPLv2 only precludes using it with
the {,L}LGPLv3 libraries from Samba, which is unfortunate.

2. BuildRequires userspace-rcu-devel >= 0.6.6

-BuildRequires:  pkgconfig libuuid-devel userspace-rcu-devel texinfo gcc-c++
systemtap-sdt-devel
+BuildRequires:  libuuid-devel texinfo systemtap-sdt-devel
+BuildRequires:  userspace-rcu-devel >= 0.6.6

lttng-ust's configure.ac wants userspace-rcu >= 0.6.6 and
since the older userspace-rcu-0.4.1 has been in Fedora since Feb 2011
having this versioned build requires is easier for users rebuilding this RPM
locally.

Remove gcc-c++ from BuildRequires per
https://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2.
Also remove pkgconfig from BuildRequires since it's not actually used in the
build process.

Note that installing lttng-ust.pc doesn't require pkgconfig. The pkgconfig
related depencies in -devel would still be automatically picked up by rpmbuild.

3. Unless you're targetting EPEL5, remove the BuildRoot tag, the %clean section
and the second %defattr() in the %files -n %{name}-devel section.

Per https://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag:

-BuildRoot:      %{_tmppath}/%{name}-%{version}-build

-%clean
-rm -rf %buildroot

 %files -n %{name}-devel
-%defattr(-,root,root)
 %{_bindir}/lttng-gen-tp

The last point in particular is mostly to make the fedora-review tool happy.

4. ExclusiveArch and arm
>From a quick reading of configure.ac and the git logs, I believe lttng-ust does
support the ARM architecture. I recommend just removing the ExclusiveArch line
unless you have a good reason.

5. I recommend adding a comment in the .spec on why "--with-java-jdk" isn't
enabled: 

 %configure --docdir=%{_docdir}/%{name} --disable-static --with-sdt

+# --with-java-jdk
+# Java support was disabled in lttng-ust's stable-2.0 branch upstream in
+#
http://git.lttng.org/?p=lttng-ust.git;a=commit;h=655a0d112540df3001f9823cd3b331b8254eb2aa
+# We can revisit enabling this when the next major version is released.
+
 make %{?_smp_mflags} V=1

6. Use quotes consistently for %buildroot like other RPM directory name macros

 %install
-make DESTDIR=%buildroot install
-rm -vf %buildroot%{_libdir}/*.la
+make DESTDIR=%{buildroot} install
+rm -vf %{buildroot}%{_libdir}/*.la

 %post -p /sbin/ldconfig
 %postun -p /sbin/ldconfig

7. Upstream recently released lttng-ust-2.0.4
I looked at the git log and suspect we'd want to pick up the various deadlock
fixes.


I'll approve this review once the above points are addressed.

On a separate note, while performing this reivew I tried to run the code in the
tests/ directory (the few that were not disabled upstream) and the example
shipped as documentation.  But since we don't lttng-tools and the log viewers
packaged, I was only able to verify that the sample and test programs can be
built and run but can't actually see the traces.

Do you intend to package lttng-tools nad the log viewers?

-- 
You are receiving this mail because:
You are on the CC list for the bug.
_______________________________________________
package-review mailing list
package-review@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/package-review



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