Bug 717748

I've an new SRPMS: 
http://www.dorsal.polymtl.ca/~ybrosseau/fedora/SRPMS/ust-0.14-2.fc15.src.rpm

Have you consider putting UST in the Fedora 16 features, so that it is advertised in the release notes?  You still have almost two weeks.

See http://fedoraproject.org/wiki/Features/Policy for more information on the process, it is lighter-weight than it seems! ;)

Thanks, I'll look into it.

A small update here:

SPEC: http://www.dorsal.polymtl.ca/~ybrosseau/fedora/SPECS/ust.spec
SRPM: http://www.dorsal.polymtl.ca/~ybrosseau/fedora/SRPMS/ust-0.14-3.fc15.src.rpm

A first brief look:


> License:	LGPLv2

File 'COPYING' and the source file headers explicitly mention "any later version", so:

License: LGPLv2+


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

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


> Requires:	liburcu >= 0.6.2

https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires


> %package -n libust-devel
> ...
> Requires:	liburcu-devel
> Requires:	libust = %{version}-%{release}
> Requires:	libustinstr-malloc = %{version}-%{release}
> Requires:	libustfork = %{version}-%{release}
> Requires:	libustconsumer = %{version}-%{release}
> Requires:	libustctl = %{version}-%{release}

https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package


> %configure --disable-silent-rules --disable-dependency-tracking
> ...
> make CFLAGS="$RPM_OPT_FLAGS" %{?_smp_mflags}

Since %configure already passes on the CFLAGS (see 'rpm --eval %configure'), is the extra CFLAGS definition when running Make necessary?


> %clean
> rm -rf %buildroot

https://fedoraproject.org/wiki/Packaging:Guidelines#.25clean


> %post -n ust
> /sbin/ldconfig
> /usr/sbin/install-info /usr/share/doc/ust.info.gz
> %preun -n ust
> /usr/sbin/install-info --delete /usr/share/doc/ust.info.gz

https://fedoraproject.org/wiki/Packaging:Guidelines#Scriptlets


> %{_libdir}/libust.a

https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries


> %{_datadir}/info/ust.info.gz

There also is %{_infodir} which is set by %configure, too.


> %files

Sometimes you use -n to explicitly specify the full package name (e.g. %post -n ust), here you don't. Then you also omit the -n for the base package's %post, %preun and %postun scriptlet sections for consistency.


> https://fedoraproject.org/wiki/Packaging:Guidelines#Documentation

Files COPYING, README and TODO could be included.


It seems the test suite currently cannot be run. If it could be run during the build process, it would make sense to create a %check section for it.

Here's an updated version following your comments. (With a new upstream release)

SPEC: http://www.dorsal.polymtl.ca/~ybrosseau/fedora/SPECS/ust.spec
SRPM:
http://www.dorsal.polymtl.ca/~ybrosseau/fedora/SRPMS/ust-0.15-1.fc15.src.rpm

A new version will be released soon. I'll post a new package then...

Updating this request with the latest LTTng-UST package.

SPEC: http://www.dorsal.polymtl.ca/~ybrosseau/fedora/SPECS/lttng-ust.spec
SRPMS: http://www.dorsal.polymtl.ca/~ybrosseau/fedora/SRPMS/lttng-ust-2.0.3-1.fc17.src.rpm

(In reply to comment #8)

Building on F17 fails for me with:

/bin/sh ../../libtool  --tag=CC   --mode=link gcc  -Wall -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4  -m64 -mtune=generic  -Wl,-z,relro  -o hello hello.o tp.o ../../liblttng-ust/liblttng-ust.la -lurcu-bp -lurcu-bp -luuid -lpthread -ldl
libtool: link: DIE_RPATH_DIE="/usr/lib64:" gcc -Wall -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic -Wl,-z -Wl,relro -o .libs/hello hello.o tp.o  ../../liblttng-ust/.libs/liblttng-ust.so -lurcu-bp -luuid -lpthread -ldl
/usr/bin/ld: warning: liblttng-ust-tracepoint.so.0, needed by ../../liblttng-ust/.libs/liblttng-ust.so, not found (try using -rpath or -rpath-link)
../../liblttng-ust/.libs/liblttng-ust.so: undefined reference to `init_tracepoint'
../../liblttng-ust/.libs/liblttng-ust.so: undefined reference to `__tracepoint_probe_register'
../../liblttng-ust/.libs/liblttng-ust.so: undefined reference to `exit_tracepoint'
../../liblttng-ust/.libs/liblttng-ust.so: undefined reference to `__tracepoint_probe_unregister'
collect2: error: ld returned 1 exit status
make[3]: *** [hello] Error 1
make[3]: Leaving directory `/home/scottt/work/lttng/fedora/lttng-ust-2.0.3/tests/hello'

Since:
1. ld is performing a non-shared, non relocatable link for .libs/hello 
2. We patched libtool to get rid of RPATH
3. hello.o requires liblttng-ust.so which in turn requires liblttng-ust-tracepoint.so.0
We need to either pass the directory containing liblttng-ust-tracepoint.so.0 as -rpath-link or specify liblttng-ust-tracepoint.so as an input to ld.

The following snippet in lttnt-ust.spec would accomplish the later:

+# link all dependent libs
+#sed -i 's|^link_all_deplibs=no|link_all_deplibs=unknown|g' libtool
+
+make %{?_smp_mflags} V=1

 %install
 make DESTDIR=%buildroot install

Yannick, could you build actually build without a patch to this effect?

I was able to build it without the patch, but now I'm trying with koji and targeting F17 and it fail there. I'll investigate and post a new version.

(In reply to comment #9)
> (In reply to comment #8)
> 
> Building on F17 fails for me with:
> 

> +# link all dependent libs
> +#sed -i 's|^link_all_deplibs=no|link_all_deplibs=unknown|g' libtool
> +
> +make %{?_smp_mflags} V=1
> 
>  %install
>  make DESTDIR=%buildroot install
> 
> Yannick, could you build actually build without a patch to this effect?

I've updated the files, can you try again?
Now the package reinitialize the libtool script instead of doing a series of sed.

(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?

(In reply to comment #12)
> (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.)

Yes, I also think that could have been the problem. I did a complete clean of lttng-ust before redoing this package. 

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

OK
Do you think it would be more appropriate to put it into a separate package? 

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

From https://www.gnu.org/licenses/gpl-faq.html#AllCompatibility
I don't think its a problem to use LTTng-Ust with LGPLV3 application or lib. 

 
> 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

Good point, forgot that one. 

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

OK

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

I'll review the list of supported arch and replace it by an explicit ExcludeArch if necessary. 

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

OK

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

OK

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

Yes, good catch, we want those 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?

Yes, I intend to do lttng-tools right after finishing LTTng-UST. 
I plan to do babeltrace soon after, when the 1.0 release is done. (Which should be in a couple of weeks max.)

(In reply to comment #13)

> > 1. License should be "LGPLv2 and GPLv2"
> OK
> Do you think it would be more appropriate to put it into a separate package? 

No, I think the existing arrangement is fine. There are many packages with "LGPLxx and GPLxx" in the license field in Fedora. I just always want to make a good faith effort on documenting the license during package review.

> > 4. ExclusiveArch and arm
> I'll review the list of supported arch and replace it by an explicit
> ExcludeArch if necessary. 

ok.

For your convenience, I've uploaded the .spec with my recommended changes here: http://scottt.tw/fedora/lttng-ust.spec
After you update your .spec I'll do a quick diff against that and I think we're good.

(In reply to comment #14)

New version:

SPEC: http://www.dorsal.polymtl.ca/~ybrosseau/fedora/SPECS/lttng-ust.spec
SRPMS: http://www.dorsal.polymtl.ca/~ybrosseau/fedora/SRPMS/lttng-ust-2.0.3-1.fc17.src.rpm

I've updated to 2.0.4. 
Was also missing a reference to MIT licence for some headers.

(In reply to comment #15)
> (In reply to comment #14)
> 
> New version:
> 
> SPEC: http://www.dorsal.polymtl.ca/~ybrosseau/fedora/SPECS/lttng-ust.spec
> SRPMS:
> http://www.dorsal.polymtl.ca/~ybrosseau/fedora/SRPMS/lttng-ust-2.0.3-1.fc17.
> src.rpm

Sorry, previous URL for SRPM was wrong...

SPEC: http://www.dorsal.polymtl.ca/~ybrosseau/fedora/SPECS/lttng-ust.spec
SRPMS: http://www.dorsal.polymtl.ca/~ybrosseau/fedora/SRPMS/lttng-ust-2.0.4-1.fc17.src.rpm

(In reply to comment #16)
Yannick, in your latest .spe, userspace-rcu-devel is listed twice in BuildRequires.

Formal review assuming that fixed follows:

Package Review
==============

Key:
- = N/A
x = Pass
! = Fail
? = Not evaluated



==== C/C++ ====
[x]: MUST Header files in -devel subpackage, if present.
[x]: MUST ldconfig called in %post and %postun if required.
[x]: MUST Package does not contain any libtool archives (.la)
[x]: MUST Package does not contain kernel modules.
[x]: MUST Package contains no static executables.
[x]: MUST Rpath absent or only used for internal libs.
[x]: MUST Package is not relocatable.
[x]: MUST Development (unversioned) .so files in -devel subpackage, if
     present.


==== Generic ====
[x]: MUST Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: MUST Package successfully compiles and builds into binary rpms on at
     least one supported primary architecture.
[x]: MUST %build honors applicable compiler flags or justifies otherwise.
[x]: MUST All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[x]: MUST Buildroot is not present
     Note: Unless packager wants to package for EPEL5 this is fine
[x]: MUST Package contains no bundled libraries.
[x]: MUST Changelog in prescribed format.
[x]: MUST Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: Clean would be needed if support for EPEL is required
[x]: MUST Sources contain only permissible code or content.
[x]: MUST Each %files section contains %defattr if rpm < 4.4
[x]: MUST Macros in Summary, %description expandable at SRPM build time.
[x]: MUST Package requires other packages for directories it uses.
[x]: MUST Package uses nothing in %doc for runtime.
[x]: MUST Package is not known to require ExcludeArch.
[x]: MUST Permissions on files are set properly.
[x]: MUST Package does not contain duplicates in %files.
[x]: MUST Fully versioned dependency in subpackages, if present.
[x]: MUST Spec file lacks Packager, Vendor, PreReq tags.
[x]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf would be needed if support for EPEL5 is required
[x]: MUST Large documentation files are in a -doc subpackage, if required.
[x]: MUST If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %doc.
[x]: MUST License field in the package spec file matches the actual license.
[x]: MUST License file installed when any subpackage combination is installed.
[x]: MUST Package consistently uses macros (instead of hard-coded directory
     names).
[x]: MUST Package is named according to the Package Naming Guidelines.
[x]: MUST Package does not generate any conflict.
[x]: MUST Package obeys FHS, except libexecdir and /usr/target.
[x]: MUST Package must own all directories that it creates.
[x]: MUST Package does not own files or directories owned by other packages.
[x]: MUST Package installs properly.
[-]: MUST Package requires pkgconfig, if .pc files are present. (EPEL5)
     Note: Only applicable for EL-5
[x]: MUST Requires correct, justified where necessary.
[x]: MUST Rpmlint output is silent.

rpmlint lttng-ust-2.0.3-1.fc18.src.rpm

lttng-ust.src: W: spelling-error %description -l en_US tracepoints -> trace points, trace-points, contraception
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

rpmlint lttng-ust-debuginfo-2.0.3-1.fc18.i686.rpm

1 packages and 0 specfiles checked; 0 errors, 0 warnings.


rpmlint lttng-ust-2.0.3-1.fc18.i686.rpm

lttng-ust.i686: W: spelling-error %description -l en_US tracepoints -> trace points, trace-points, contraception
lttng-ust.i686: W: shared-lib-calls-exit /usr/lib/liblttng-ust-ctl.so.0.0.0 exit
1 packages and 0 specfiles checked; 0 errors, 2 warnings.

rpmlint lttng-ust-devel-2.0.3-1.fc18.i686.rpm

lttng-ust-devel.i686: W: spelling-error %description -l en_US userspace -> user space, user-space, users pace
1 packages and 0 specfiles checked; 0 errors, 1 warnings.

The "tracepoint" should be allowed in the description.
liblttng-ust-ctl calling exit() is fine.


[x]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
  MD5SUM this package     : ff336f47ff54e8147bd892940b495785
  MD5SUM upstream package : ff336f47ff54e8147bd892940b495785

[x]: MUST Spec file is legible and written in American English.
[x]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[-]: MUST Package contains a SysV-style init script if in need of one.
[x]: MUST File names are valid UTF-8.
[x]: MUST Useful -debuginfo package or justification otherwise.
[x]: SHOULD Reviewer should test that the package builds in mock.
[x]: SHOULD If the source package does not include license text(s) as a
     separate file from upstream, the packager SHOULD query upstream to
     include it.
[x]: SHOULD Dist tag is present.
[x]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin,
     /usr/sbin.
[x]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[x]: SHOULD Package functions as described.
[x]: SHOULD Latest version is packaged.
[x]: SHOULD Package does not include license text files separate from
     upstream.
[x]: SHOULD The placement of pkgconfig(.pc) files are correct.
[x]: SHOULD Scriptlets must be sane, if used.
[x]: SHOULD SourceX is a working URL.
[x]: SHOULD Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: SHOULD Package should compile and build into binary rpms on all supported
     architectures.

http://koji.fedoraproject.org/koji/taskinfo?taskID=4178203

[x]: SHOULD %check is present and all tests pass.
[x]: SHOULD Packages should try to preserve timestamps of original installed
     files.
[x]: SHOULD Spec use %global instead of %define.

Approved.

(In reply to comment #17)
> (In reply to comment #16)
> Yannick, in your latest .spe, userspace-rcu-devel is listed twice in
> BuildRequires.

Fixed. 

> lttng-ust-devel.i686: W: spelling-error %description -l en_US userspace ->
> user space, user-space, users pace
> 1 packages and 0 specfiles checked; 0 errors, 1 warnings.
> 
> The "tracepoint" should be allowed in the description.
> liblttng-ust-ctl calling exit() is fine.

And I filed a bug report upstream to ask if its possible to get rid of it. 
https://bugs.lttng.org/issues/267

> 
> Approved.

Thanks!

(In reply to comment #18)

I think you meant to change the "fedora-cvs" flag to "?" and not the "fedora-review" flag (which should stay at "+" for a successful review).

You'll want to post a "new package scm request" snippet formatted like those on  http://fedoraproject.org/wiki/Package_SCM_admin_requests#New_Packages

New Package SCM Request
=======================
Package Name: lttng-ust
Short Description: LTTng Userspace Tracer library
Owners: greenscientist
Branches: f17 el6
InitialCC:

(In reply to comment #19)
> (In reply to comment #18)
> 
> I think you meant to change the "fedora-cvs" flag to "?" and not the
> "fedora-review" flag (which should stay at "+" for a successful review).
> 
> You'll want to post a "new package scm request" snippet formatted like those
> on  http://fedoraproject.org/wiki/Package_SCM_admin_requests#New_Packages

Yes, I think I miss handled something when I have answered you.

SCM request name and Summary name don't match, please correct.

Git done (by process-git-requests).

For those interested, I've posted a review request for LTTng-tools in bug #834481

lttng-ust-2.0.4-2.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/lttng-ust-2.0.4-2.fc17

lttng-ust-2.0.4-2.fc17 has been pushed to the Fedora 17 testing repository.

lttng-ust-2.0.4-2.fc17 has been pushed to the Fedora 17 stable repository.

lttng-ust-2.0.4-2.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/lttng-ust-2.0.4-2.el6

lttng-ust-2.0.4-2.el6 has been pushed to the Fedora EPEL 6 stable repository.

I am also getting the same error as in comment.
I am using lttng .216 on linux 2.6.33.7.
I tried the solution in site https://bugs.lttng.org/issues/321
but it did not help.
Any way out of this issue?