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.

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

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.

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

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

FYI, a link to a 6 minute lightning talk by the author about uftrace:
https://youtu.be/LNav5qvyK7I

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

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.

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

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

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?

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.

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

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

A copr repository is available here: https://copr.fedorainfracloud.org/coprs/bkircher/uftrace/

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.

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

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

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.

Thanks, Dridi.

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

I'll do my best :)

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

*** Bug 1525268 has been marked as a duplicate of this bug. ***

I'm not interested in the package anymore