[Bug 1398922] Review Request: uftrace - User-space function call tracer for C and C++ programs

[bugzilla]

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

Benjamin Kircher  changed:

   What|Removed |Added

 Status|NEW |CLOSED
 Resolution|--- |NOTABUG
Last Closed||2019-01-10 10:51:18



--- Comment #23 from Benjamin Kircher  ---
I'm not interested in the package anymore

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org
Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives: 
https://lists.fedoraproject.org/archives/list/package-review@lists.fedoraproject.org

[Bug 1398922] Review Request: uftrace - User-space function call tracer for C and C++ programs

[bugzilla]

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

Robert-André Mauchin  changed:

   What|Removed |Added

 CC||fed...@svgames.pl



--- Comment #22 from Robert-André Mauchin  ---
*** Bug 1525268 has been marked as a duplicate of this bug. ***

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1398922] Review Request: uftrace - User-space function call tracer for C and C++ programs

[bugzilla]

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

Michal Schmidt  changed:

   What|Removed |Added

 CC||mschm...@redhat.com



--- Comment #21 from Michal Schmidt  ---
(In reply to Benjamin Kircher from comment #13)
> > 1) These BR are not needed: gcc
> BR removed.

It should stay. See
https://fedoraproject.org/wiki/Packaging:C_and_C%2B%2B#BuildRequires_and_Requires
When rpmlint and the Guidelines disagree, side with the Guidelines. rpmlint is
not always up to date.

> %files
> %{_bindir}/uftrace
> %{_libdir}/%{name}/libmcount*.so

Your package should own the directory %{_libdir}/%{name} itself, not just the
file in it.

> %{_mandir}/man1/uftrace*.1*
> %{_sysconfdir}/bash_completion.d/uftrace

Please place packaged completions under
%{_datadir}/bash_completion/completions/ and keep /etc for the administrator.
The current practice is to co-own the completion directories, though I would
like to see that changed (bug #1504616).

> * Wed Jun 28 2017 Benjamin Kircher  - 0.7-1
> - New upstream release
> - Use %configure macro

Beware of macro expansion inside the changelog. Use double percent signs to
prevent expansion, like this:
  - Use %%configure macro

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1398922] Review Request: uftrace - User-space function call tracer for C and C++ programs

[bugzilla]

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



--- Comment #20 from Benjamin Kircher  ---
Thanks, Dridi.

> Don't forget to find a sponsor in the mean time.

I'll do my best :)

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1398922] Review Request: uftrace - User-space function call tracer for C and C++ programs

[bugzilla]

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



--- Comment #19 from Dridi Boukelmoune  ---
I have some packaging work ahead, I'll have a look at your update then. Don't
forget to find a sponsor in the mean time. Maybe I can do the formal review,
but I can't approve it.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1398922] Review Request: uftrace - User-space function call tracer for C and C++ programs

[bugzilla]

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



--- Comment #18 from Benjamin Kircher  ---
Thanks Dridi for your work. New upstream version contains your changes so no
need to patch.

Changes:
- New upstream release 0.7
- Use %configure macro
- Do verbose build

New SPEC file:
https://raw.githubusercontent.com/bkircher/uftrace-fedora/master/uftrace.spec

New SRPM:
https://raw.githubusercontent.com/bkircher/uftrace-fedora/master/uftrace-0.7-1.fc25.src.rpm

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1398922] Review Request: uftrace - User-space function call tracer for C and C++ programs

[bugzilla]

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

Dridi Boukelmoune  changed:

   What|Removed |Added

 CC||dridi.boukelmo...@gmail.com



--- Comment #17 from Dridi Boukelmoune  ---
The next release should work fine with the %configure macro [1] so that should
soon not be a problem. If you think that the shared object should be in a
private directory because they're not directly linked to but rather pre-loaded
you should discuss this upstream and point them to the relevant Fedora
guidelines.

You can however do that in your spec like this:

%configure --libdir=%{_libdir}/%{name}

Until the next release, you can patch [2,3] the configure script in your spec.

As upstream mentioned on github, I was about to recommend verbose building to

1) verify that the flags are properly passed and
2) make it easier to troubleshoot remote builds in general:

%make_build V=1

There are other issues with the spec, have a look at the rpmlint output for
starters. I found one in the changelog that I'm sure rpmlint will report, I'll
keep the others for later.

[1] https://github.com/namhyung/uftrace/pull/98
[2] https://patch-diff.githubusercontent.com/raw/namhyung/uftrace/pull/98.patch
[3] https://patch-diff.githubusercontent.com/raw/namhyung/uftrace/pull/98.diff

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1398922] Review Request: uftrace - User-space function call tracer for C and C++ programs

[bugzilla]

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



--- Comment #16 from Benjamin Kircher  ---
FYI, some changes related to ./configure script and Fedora packaging happened
in upstream here:
https://github.com/namhyung/uftrace/pull/98

Not in a release yet, though.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1398922] Review Request: uftrace - User-space function call tracer for C and C++ programs

[bugzilla]

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



--- Comment #15 from Benjamin Kircher  ---
A copr repository is available here:
https://copr.fedorainfracloud.org/coprs/bkircher/uftrace/

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1398922] Review Request: uftrace - User-space function call tracer for C and C++ programs

[bugzilla]

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



--- Comment #14 from Benjamin Kircher  ---
New upstream version.

New SPEC file:
https://raw.githubusercontent.com/bkircher/uftrace-fedora/master/uftrace.spec

New SRPM:
https://raw.githubusercontent.com/bkircher/uftrace-fedora/master/uftrace-0.6.2-1.fc25.src.rpm

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1398922] Review Request: uftrace - User-space function call tracer for C and C++ programs

[bugzilla]

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



--- Comment #13 from Benjamin Kircher  ---
Sorry for letting this lie around for 9 weeks.

> About GPLv2 or GPLv2+
You're right. It is GPLv2. Author confirmed this.

> 1) These BR are not needed: gcc
BR removed.

> I'd rather adjust the INSTALL_LIB_PATH definition before compiling uftrace 
> and install them into a private path below $libdir
Done exactly so.


Additional changes:

- added %check section
- new upstream release
- bash completion script is installed to /etc/bash_completion.d
- add `ExclusiveArch:`, uftrace only supports x86_64 and ARM (v6,7) for now


New SPEC file:
https://raw.githubusercontent.com/bkircher/uftrace-fedora/master/uftrace.spec

New SRPM:
https://raw.githubusercontent.com/bkircher/uftrace-fedora/master/uftrace-0.6.1-2.fc25.src.rpm

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1398922] Review Request: uftrace - User-space function call tracer for C and C++ programs

[bugzilla]

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



--- Comment #12 from Michael Schwendt  ---
The fedora-review tool is equally helpful to the package maintainer as to the
reviewer.


About GPLv2 or GPLv2+, a simple grep turns up these false positives:

  $ grep "or later" * -R
  libtraceevent/trace-seq.c: * into a special buffer (@s) for later retrieval
by a sequencer
  libtraceevent/event-parse.c:/* Save for later use. */


> 1) These BR are not needed: gcc

Wouldn't be a big issue. The default buildroot doesn't contain gcc-c++ anymore,
however. So, if gcc were not found, you would notice that with a failing build.


> 2) Unversioned so-files not in -devel subpackage.

They are dlopen()ed plugins. I'd rather adjust the INSTALL_LIB_PATH definition
before compiling uftrace and install them into a private path below $libdir
instead of directly into $libdir.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1398922] Review Request: uftrace - User-space function call tracer for C and C++ programs

[bugzilla]

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



--- Comment #11 from Benjamin Kircher  ---
Hi Michael. Thank you for looking into this.

> > * I think license is GPLv2+, not GPLv2
>
> It's useless to write that without giving a rationale.
>
> Have you pointed "fedora-review -b 1398922" at this ticket yet to see which
> test results it comes up with?
Not sure if you asked Igor or me here  (Igor, I presume). I don't want to get
ahead of things here but anyway: Yes I did just now.

rpmlint reports 3 errors (incorrect-fsf-address) and lots of warnings
(partially useful) i.e., about those unversioned so-files in the package.

For your convenience, please find results here:
https://raw.githubusercontent.com/bkircher/uftrace-fedora/master/review.txt

Things I should fix (I guess, waiting for input from you guys here):

1) These BR are not needed: gcc
2) Unversioned so-files not in -devel subpackage. Should we open a ticket with
Fedora Packaging Committee as per
https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#DevelPackages
because of that?

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1398922] Review Request: uftrace - User-space function call tracer for C and C++ programs

[bugzilla]

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



--- Comment #10 from Benjamin Kircher  ---
New SPEC file:
https://raw.githubusercontent.com/bkircher/uftrace-fedora/master/uftrace.spec

New SRPM:
https://raw.githubusercontent.com/bkircher/uftrace-fedora/master/uftrace-0.6-3.fc25.src.rpm

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1398922] Review Request: uftrace - User-space function call tracer for C and C++ programs

[bugzilla]

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



--- Comment #9 from Michael Schwendt  ---
> * I think license is GPLv2+, not GPLv2

It's useless to write that without giving a rationale.

Have you pointed "fedora-review -b 1398922" at this ticket yet to see which
test results it comes up with?

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1398922] Review Request: uftrace - User-space function call tracer for C and C++ programs

[bugzilla]

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



--- Comment #8 from Benjamin Kircher  ---
Thank you.

> * ./configure  -> %configure
> Usually, but unfortunately it's not the case right now.
Correct, configure is a shell script here.

> Main problem is that CFLAGS/LDFLAGS are ignored.
> you can do something like:
> export CFLAGS="%{__global_cflags}"
> exprot LDFLAGS="%{__global_ldflags}" before configure call
Fixed.

> * rm -rf $RPM_BUILD_ROOT is not needed
Fixed.

> * I think license is GPLv2+, not GPLv2
Not 100% sure on this one. The license header used in the source files
explicitly states version 2. The usual "either version 2 of the License, or (at
your option) any later version" is missing here in the text. I ask the author
to clarify this.

> * Missing BuildRequires: gcc
Fixed.

> * I would recommend make URL without macro, so it will be easily clickable
Makes sense. Followed your recommendation.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1398922] Review Request: uftrace - User-space function call tracer for C and C++ programs

[bugzilla]

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

Igor Gnatenko  changed:

   What|Removed |Added

 Blocks||177841 (FE-NEEDSPONSOR)



--- Comment #7 from Igor Gnatenko  ---
* ./configure --prefix=%{_prefix} --libdir=%{_libdir} -> %configure
Usually, but unfortunately it's not the case right now. Main problem is that
CFLAGS/LDFLAGS are ignored.
you can do something like:
export CFLAGS="%{__global_cflags}"
exprot LDFLAGS="%{__global_ldflags}" before configure call
* rm -rf $RPM_BUILD_ROOT is not needed
* I think license is GPLv2+, not GPLv2
* Missing BuildRequires: gcc
* I would recommend make URL without macro, so it will be easily clickable

Otherwise, looks good from first glance.

Unfortunately you are not in packagers group, so you need a sponsor.
Looks like you need to start from
https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Introduce_yourself
point.


Referenced Bugs:

https://bugzilla.redhat.com/show_bug.cgi?id=177841
[Bug 177841] Tracker: Review requests from new Fedora packagers who need a
sponsor
-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1398922] Review Request: uftrace - User-space function call tracer for C and C++ programs

[bugzilla]

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



--- Comment #6 from Benjamin Kircher  ---
FYI, a link to a 6 minute lightning talk by the author about uftrace:
https://youtu.be/LNav5qvyK7I

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1398922] Review Request: uftrace - User-space function call tracer for C and C++ programs

[bugzilla]

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



--- Comment #5 from Benjamin Kircher  ---
> Sorry, as a new packager myself, I misunderstood the rules about .so files.
Oh, no problem. The question what these libraries are doing is quite justified.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1398922] Review Request: uftrace - User-space function call tracer for C and C++ programs

[bugzilla]

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



--- Comment #4 from Yunying Sun  ---
(In reply to Benjamin Kircher from comment #2)
> Thank you.
> 
> > 1. No version specified in %changelog.
> Fixed.
> 
> > Static libraries are discouraged. Add "--disable-static" in %configure, and 
> > add -devel package where *.so could be included.
> There are no static libraries in this package. The libmcount*.so files are
> actually needed to run the application and get loaded by the `uftrace`
> binary dynamically at run-time. I don't think this is really a plugin system
> but seems to work kind of the same way.
> 
Sorry, as a new packager myself, I misunderstood the rules about .so files.
There're no versioned shared library files in this package, so the unversioned
.so should be included in the base package. And since it has no static libs, no
header files, no need for a -devel package.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1398922] Review Request: uftrace - User-space function call tracer for C and C++ programs

[bugzilla]

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



--- Comment #3 from Benjamin Kircher  ---
I was able to talk to the author of uftrace about those libs. The libmcount*
libraries are linked into the target process (using LD_PRELOAD) rather than
uftrace itself. They do the all the work to generate trace data and talk to
uftrace.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1398922] Review Request: uftrace - User-space function call tracer for C and C++ programs

[bugzilla]

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



--- Comment #2 from Benjamin Kircher  ---
Thank you.

> 1. No version specified in %changelog.
Fixed.

> Static libraries are discouraged. Add "--disable-static" in %configure, and 
> add -devel package where *.so could be included.
There are no static libraries in this package. The libmcount*.so files are
actually needed to run the application and get loaded by the `uftrace` binary
dynamically at run-time. I don't think this is really a plugin system but seems
to work kind of the same way.

> 3. Missing %doc for README.md.
Fixed.

> 4. Use %{name} instead of fixed string "uftrace" would be better in spec file.
Fixed.

New SPEC file:
https://raw.githubusercontent.com/bkircher/uftrace-fedora/master/uftrace.spec

New SRPM:
https://raw.githubusercontent.com/bkircher/uftrace-fedora/master/uftrace-0.6-2.fc25.src.rpm

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org

[Bug 1398922] Review Request: uftrace - User-space function call tracer for C and C++ programs

[bugzilla]

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

Yunying Sun  changed:

   What|Removed |Added

 CC||yunying@intel.com



--- Comment #1 from Yunying Sun  ---
An unformal review:

1. No version specified in %changelog.
changelog format:
https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs

2. Static libraries are discouraged. Add "--disable-static" in %configure, and
add -devel package where *.so could be included.
Refer to:
https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries
https://fedoraproject.org/wiki/Packaging:Guidelines#DevelPackages

3. Missing %doc for README.md.

4. Use %{name} instead of fixed string "uftrace" would be better in spec file.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list -- package-review@lists.fedoraproject.org
To unsubscribe send an email to package-review-le...@lists.fedoraproject.org