[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 #13 from Yannick Brosseau <yannick.brosseau@xxxxxxxxx> ---
(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.)

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