Bug 1369708

> BuildRequires:  pkgconfig
> BuildRequires:  libcmocka-devel
BuildRequires: pkgconfig(cmocka)

> CONFIG_SITE=$(pwd)/lib/default_config.site ./configure
export CONFIG_SITE=$(pwd)/lib/default_config.site
%configure

> make %{?_smp_mflags}
%make_build

* Missing BuildRequires: gcc
* Redundant rm -rf $RPM_BUILD_ROOT
* Missing -devel subpackage which contains headers and unversioned .so
* Globs are too relax (/me guessing package contains .la and .a files)

(In reply to Igor Gnatenko from comment #1)
> > BuildRequires:  pkgconfig
> > BuildRequires:  libcmocka-devel
> BuildRequires: pkgconfig(cmocka)
> 
> > CONFIG_SITE=$(pwd)/lib/default_config.site ./configure
> export CONFIG_SITE=$(pwd)/lib/default_config.site
> %configure
> 
> > make %{?_smp_mflags}
> %make_build
> 
> * Missing BuildRequires: gcc
> * Redundant rm -rf $RPM_BUILD_ROOT
> * Missing -devel subpackage which contains headers and unversioned .so
> * Globs are too relax (/me guessing package contains .la and .a files)

Thanks Igor, for your quick review and comments.
I will update spec accordingly.

(In reply to Igor Gnatenko from comment #1)
> > BuildRequires:  pkgconfig
> > BuildRequires:  libcmocka-devel
> BuildRequires: pkgconfig(cmocka)
> 
> > CONFIG_SITE=$(pwd)/lib/default_config.site ./configure
> export CONFIG_SITE=$(pwd)/lib/default_config.site
> %configure
> 
> > make %{?_smp_mflags}
> %make_build
> 
> * Missing BuildRequires: gcc
> * Redundant rm -rf $RPM_BUILD_ROOT
> * Missing -devel subpackage which contains headers and unversioned .so
> * Globs are too relax (/me guessing package contains .la and .a files)

Hi Igor, 

I updated SPEC file following your comments:
https://github.com/yunyings/share/blob/master/tpm2-tss.spec
https://github.com/yunyings/share/blob/master/tpm2-tss-1.0-0.1.beta1.el7.src.rpm

Local rpmbuild completed successfully. But Koji build failed:
http://koji.fedoraproject.org/koji/taskinfo?taskID=15435887

error details:
checking whether we are cross compiling... configure: error: in `/builddir/build/BUILD/tpm2-tss-1.0':
configure: error: cannot run C compiled programs.
If you meant to cross compile, use `--host'.
See `config.log' for more details

No config.log found on koji server. Local config.log doesn't tell much.
Could you help on this?  Thanks.

the configure error "cannot run C compiled programs" is there, seems because "%configure" adds an extra flag "-Werror" to CFLAGS, which turns some config warnings to errors. With "./configure", koji build pass.

@Igor, is it acceptable to use:
CONFIG_SITE=$(pwd)/lib/default_config.site ./configure
#%configure

instead of:
export CONFIG_SITE=$(pwd)/lib/default_config.site
%configure

I've verified that koji build pass if comment off "%configure".
updated SPEC: https://github.com/yunyings/share/blob/master/tpm2-tss.spec
koji rawhide build: http://koji.fedoraproject.org/koji/taskinfo?taskID=15448486

(In reply to yunying.sun from comment #4)
> the configure error "cannot run C compiled programs" is there, seems because
> "%configure" adds an extra flag "-Werror" to CFLAGS, which turns some config
> warnings to errors. With "./configure", koji build pass.
> 
> @Igor, is it acceptable to use:
> CONFIG_SITE=$(pwd)/lib/default_config.site ./configure
> #%configure
> 
> instead of:
> export CONFIG_SITE=$(pwd)/lib/default_config.site
> %configure
> 
> I've verified that koji build pass if comment off "%configure".
> updated SPEC: https://github.com/yunyings/share/blob/master/tpm2-tss.spec
> koji rawhide build:
> http://koji.fedoraproject.org/koji/taskinfo?taskID=15448486

not really, because then it (I suppose) will ignore Fedora CFLAGS/LDFLAGS/CXXFLAGS/....

From your koji build it looks like %configure was executed and it worked.

(In reply to Igor Gnatenko from comment #5)
> (In reply to yunying.sun from comment #4)
> > the configure error "cannot run C compiled programs" is there, seems because
> > "%configure" adds an extra flag "-Werror" to CFLAGS, which turns some config
> > warnings to errors. With "./configure", koji build pass.
> > 
> > @Igor, is it acceptable to use:
> > CONFIG_SITE=$(pwd)/lib/default_config.site ./configure
> > #%configure
> > 
> > instead of:
> > export CONFIG_SITE=$(pwd)/lib/default_config.site
> > %configure
> > 
> > I've verified that koji build pass if comment off "%configure".
> > updated SPEC: https://github.com/yunyings/share/blob/master/tpm2-tss.spec
> > koji rawhide build:
> > http://koji.fedoraproject.org/koji/taskinfo?taskID=15448486
> 
> not really, because then it (I suppose) will ignore Fedora
> CFLAGS/LDFLAGS/CXXFLAGS/....
> 
> From your koji build it looks like %configure was executed and it worked.

@Igor, the koji build succeeded in comment 4 because "./configure" is used instead of "%configure".

With:
CONFIG_SITE=$(pwd)/lib/default_config.site ./configure
#%configure
Koji build succeeded: http://koji.fedoraproject.org/koji/taskinfo?taskID=15459008

With:
export CONFIG_SITE=$(pwd)/lib/default_config.site
%configure
koji build failed: http://koji.fedoraproject.org/koji/taskinfo?taskID=15458924

(In reply to yunying.sun from comment #6)
> (In reply to Igor Gnatenko from comment #5)
> > (In reply to yunying.sun from comment #4)
> > > the configure error "cannot run C compiled programs" is there, seems because
> > > "%configure" adds an extra flag "-Werror" to CFLAGS, which turns some config
> > > warnings to errors. With "./configure", koji build pass.
> > > 
> > > @Igor, is it acceptable to use:
> > > CONFIG_SITE=$(pwd)/lib/default_config.site ./configure
> > > #%configure
> > > 
> > > instead of:
> > > export CONFIG_SITE=$(pwd)/lib/default_config.site
> > > %configure
> > > 
> > > I've verified that koji build pass if comment off "%configure".
> > > updated SPEC: https://github.com/yunyings/share/blob/master/tpm2-tss.spec
> > > koji rawhide build:
> > > http://koji.fedoraproject.org/koji/taskinfo?taskID=15448486
> > 
> > not really, because then it (I suppose) will ignore Fedora
> > CFLAGS/LDFLAGS/CXXFLAGS/....
> > 
> > From your koji build it looks like %configure was executed and it worked.
> 
> @Igor, the koji build succeeded in comment 4 because "./configure" is used
> instead of "%configure".

One correction here. So far the only working "%build" field looks like:

%build
./bootstrap
CONFIG_SITE=$(pwd)/lib/default_config.site ./configure
%configure
%make_build

That is, "./configure" has to be done before configuration comes to "%configure".

@Igor, does the "%build" above looks good to you?

> 
> With:
> CONFIG_SITE=$(pwd)/lib/default_config.site ./configure
> #%configure
> Koji build succeeded:
> http://koji.fedoraproject.org/koji/taskinfo?taskID=15459008
> 
> With:
> export CONFIG_SITE=$(pwd)/lib/default_config.site
> %configure
> koji build failed:
> http://koji.fedoraproject.org/koji/taskinfo?taskID=15458924

(In reply to yunying.sun from comment #4)
> the configure error "cannot run C compiled programs" is there, seems because
> "%configure" adds an extra flag "-Werror" to CFLAGS, which turns some config
> warnings to errors. With "./configure", koji build pass.
> 
> @Igor, is it acceptable to use:
> CONFIG_SITE=$(pwd)/lib/default_config.site ./configure
> #%configure
> 
> instead of:
> export CONFIG_SITE=$(pwd)/lib/default_config.site
> %configure

Neither is acceptable. Both ways disable checks configure performs and use cached values instead - This way you are tricking yourself by outsmarting configure.

Furthermore:

- ./bootstrap belongs into %prep not %configure

- Fedora packages must not install anything into /usr/local.
I.e. this is not allowed in fedora:
%files
%{_prefix}/local/sbin/tpm2_*

- Your src.rpm fails to build for fedora due to missing BuildRequires. 
  You'd have to submit these packages.

(In reply to Ralf Corsepius from comment #8)
> (In reply to yunying.sun from comment #4)
> > the configure error "cannot run C compiled programs" is there, seems because
> > "%configure" adds an extra flag "-Werror" to CFLAGS, which turns some config
> > warnings to errors. With "./configure", koji build pass.
> > 
> > @Igor, is it acceptable to use:
> > CONFIG_SITE=$(pwd)/lib/default_config.site ./configure
> > #%configure
> > 
> > instead of:
> > export CONFIG_SITE=$(pwd)/lib/default_config.site
> > %configure
> 
> Neither is acceptable. Both ways disable checks configure performs and use
> cached values instead - This way you are tricking yourself by outsmarting
> configure.
Thanks Ralf for your comment. 
@Ralf, if neither is acceptable, could you point the right way of configuring?

> 
> Furthermore:
> 
> - ./bootstrap belongs into %prep not %configure
Will move ./bootstrap to %prep.

> 
> - Fedora packages must not install anything into /usr/local.
> I.e. this is not allowed in fedora:
> %files
> %{_prefix}/local/sbin/tpm2_*
You must be seeing the other SPEC file - tpm2-tools.spec.
There's another bz for it: https://bugzilla.redhat.com/show_bug.cgi?id=1369720
I will fix the "/usr/local" issue then update tpm2-tools.spec.

> 
> - Your src.rpm fails to build for fedora due to missing BuildRequires. 
>   You'd have to submit these packages.
Yes, tpm2-tools.srpm fails to build due to missing BuildRequires for package "tpm2-tss" & "tpm2-tss-devel". 
To make tpm2-tools koji build PASS, does it has to wait until tpm2-tss & tpm2-tss-devel get integrated?

(In reply to Ralf Corsepius from comment #8)
> (In reply to yunying.sun from comment #4)
> > the configure error "cannot run C compiled programs" is there, seems because
> > "%configure" adds an extra flag "-Werror" to CFLAGS, which turns some config
> > warnings to errors. With "./configure", koji build pass.
> > 
> > @Igor, is it acceptable to use:
> > CONFIG_SITE=$(pwd)/lib/default_config.site ./configure
> > #%configure
> > 
> > instead of:
> > export CONFIG_SITE=$(pwd)/lib/default_config.site
> > %configure
> 
> Neither is acceptable. Both ways disable checks configure performs and use
> cached values instead - This way you are tricking yourself by outsmarting
> configure.
> 
> Furthermore:
> 
> - ./bootstrap belongs into %prep not %configure
> 
> - Fedora packages must not install anything into /usr/local.
> I.e. this is not allowed in fedora:
> %files
> %{_prefix}/local/sbin/tpm2_*
> 
> - Your src.rpm fails to build for fedora due to missing BuildRequires. 
>   You'd have to submit these packages.

@Ralf, Igor, I've updated the SPEC & SRPM again:
https://github.com/yunyings/share/blob/master/tpm2-tss.spec
https://github.com/yunyings/share/blob/master/tpm2-tss-1.0-0.1.beta1.el7.src.rpm

This time koji build pass:
http://koji.fedoraproject.org/koji/taskinfo?taskID=15461420

Please help to review again. Thanks.

(In reply to yunying.sun from comment #9)

> Yes, tpm2-tools.srpm fails to build due to missing BuildRequires for package
> "tpm2-tss" & "tpm2-tss-devel". 
> To make tpm2-tools koji build PASS, does it has to wait until tpm2-tss &
> tpm2-tss-devel get integrated?
Yes. Please submit the missing packages for review and add the corresponding BZ numbers to the "Depends On:"-field in this BZ's page (see top of this page).

(In reply to Ralf Corsepius from comment #11)
> (In reply to yunying.sun from comment #9)
> 
> > Yes, tpm2-tools.srpm fails to build due to missing BuildRequires for package
> > "tpm2-tss" & "tpm2-tss-devel". 
> > To make tpm2-tools koji build PASS, does it has to wait until tpm2-tss &
> > tpm2-tss-devel get integrated?
> Yes. Please submit the missing packages for review and add the corresponding
> BZ numbers to the "Depends On:"-field in this BZ's page (see top of this
> page).

@Ralf, thanks a lot for the review and comments. I have updated SPEC for tpm2-tss. 

SPEC url: https://github.com/yunyings/share/blob/master/tpm2-tss.spec
SRPM url: https://github.com/yunyings/share/blob/master/tpm2-tss-1.0-0.1.beta1.el7.src.rpm
Succeeded koji build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=15461420

Could you help to review again?

As for tpm2-tools(Review Request: 1369720), since it depends on this ticket to get tpm2-tss & tpm2-tss-devel integrated, currently koji build always fail. More information on 1369720. Could you help to review that one too? Thanks again.

(In reply to yunying.sun from comment #12)
> Could you help to review again?

Here we go:

- Please append --disable-silent-rules to %configure
This enforces verbose building, because silent building is not helpful when building packages in batches.

- Please disable-static libs.
Static libs are strongly discouraged in Fedora.

To achieve this, append --disable-static to %configure
and remove %{_libdir}/*.a from %files

- Please add LICENSE to %license in %files:
%files
...
%license LICENSE

- Consider to add README.md and ChangeLog to %doc
%files
...
%doc README.md ChangeLog

- *-devel should 
Requires: %{name}%{?_isa} = %{version}-%{release}
not
Requires: %{name} = %{version}-%{release}

- Directories %{_includedir}/sapi and %{_includedir}/tcti should be owned.
Please change 
%{_includedir}/sapi/*.h
%{_includedir}/tcti/*.h
into
%{_includedir}/sapi
%{_includedir}/tcti


I am going to attach a patch proposal comprising all changes up to here to this BZ.




- IMHO, the CFLAGS in upstream's *.pc.ins are bogus.
They all contain
Cflags: -I@includedir@/<subpkg>
while I think they should contain
Cflags: -I@includedir@

What actually is correct, depends upon which form of #include statements upstream expects consumers/users of these libs to use: 
If they are expected to use "#include <someheader.h>"
then the 1st form would be correct.
If they are expected to use "#include <subpkg/someheader.h>"
then the 2nd form would be correct.

In general, the 2nd form is better, because the 1st form is more likely to erroniously pickup bogus headers from the compiler's include path.


Finally, one general (upstream-related) question:
On which architectures/target-platform is this package useful on/applicable to?
I guess, it's probably x86_64 + i686 only (or x86_64 only?), but not on others (arm, sparc, s3**, ppc, ...).

Created attachment 1197361 [details]
Patch to tpm2-tss.spec as announced in comment#13

(In reply to Ralf Corsepius from comment #13)
> (In reply to yunying.sun from comment #12)
> > Could you help to review again?
> 
> Here we go:
> 
> - Please append --disable-silent-rules to %configure
> This enforces verbose building, because silent building is not helpful when
> building packages in batches.
> 
> - Please disable-static libs.
> Static libs are strongly discouraged in Fedora.
> 
> To achieve this, append --disable-static to %configure
> and remove %{_libdir}/*.a from %files
> 
> - Please add LICENSE to %license in %files:
> %files
> ...
> %license LICENSE
> 
> - Consider to add README.md and ChangeLog to %doc
> %files
> ...
> %doc README.md ChangeLog
> 
> - *-devel should 
> Requires: %{name}%{?_isa} = %{version}-%{release}
> not
> Requires: %{name} = %{version}-%{release}
> 
> - Directories %{_includedir}/sapi and %{_includedir}/tcti should be owned.
> Please change 
> %{_includedir}/sapi/*.h
> %{_includedir}/tcti/*.h
> into
> %{_includedir}/sapi
> %{_includedir}/tcti
> 
> 
> I am going to attach a patch proposal comprising all changes up to here to
> this BZ.

@Ralf, really appreciate your detailed comments and the proposal patch.
I've just updated tpm2-tss SPEC. Besides the changes you mentioned, I added one more line:
ExclusiveArch:  %{ix86} x86_64
to address the target-platform question you mentioned below.

Here is the updated SPEC & SRPM:
SPEC url: https://github.com/yunyings/share/blob/master/tpm2-tss.spec
SRPM url: https://github.com/yunyings/share/blob/master/tpm2-tss-1.0-0.1.beta1.el7.src.rpm

Succeeded koji build:
http://koji.fedoraproject.org/koji/taskinfo?taskID=15501735

Could you help to review again? Thanks a lot.

> 
> 
> 
> 
> - IMHO, the CFLAGS in upstream's *.pc.ins are bogus.
> They all contain
> Cflags: -I@includedir@/<subpkg>
> while I think they should contain
> Cflags: -I@includedir@
> 
> What actually is correct, depends upon which form of #include statements
> upstream expects consumers/users of these libs to use: 
> If they are expected to use "#include <someheader.h>"
> then the 1st form would be correct.
> If they are expected to use "#include <subpkg/someheader.h>"
> then the 2nd form would be correct.
> 
> In general, the 2nd form is better, because the 1st form is more likely to
> erroniously pickup bogus headers from the compiler's include path.
For this upstream CFLAGS issue, I will discuss with upstream developer and update here later.

> 
> 
> Finally, one general (upstream-related) question:
> On which architectures/target-platform is this package useful on/applicable
> to?
> I guess, it's probably x86_64 + i686 only (or x86_64 only?), but not on
> others (arm, sparc, s3**, ppc, ...).

(In reply to Ralf Corsepius from comment #13)
> (In reply to yunying.sun from comment #12)
> > Could you help to review again?
> 
> Here we go:
> 
> - Please append --disable-silent-rules to %configure
> This enforces verbose building, because silent building is not helpful when
> building packages in batches.
> 
> - Please disable-static libs.
> Static libs are strongly discouraged in Fedora.
> 
> To achieve this, append --disable-static to %configure
> and remove %{_libdir}/*.a from %files
> 
@Ralf, after disabling static library build, the other package tpm2-tools local rpm build fails due to not finding SAPI. Reason is that it depens on libsapi.a & libtcti-*.a which ought to be generated from tpm2-tss build.

What is the right way to solve this issue?

On wiki: https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries, one solution is put static libs into a *-static subpackage, and use "BuildRequires: *-static" for packages which links to this static subpackage.

Is this the right way to go?


> - Please add LICENSE to %license in %files:
> %files
> ...
> %license LICENSE
> 
> - Consider to add README.md and ChangeLog to %doc
> %files
> ...
> %doc README.md ChangeLog
> 
> - *-devel should 
> Requires: %{name}%{?_isa} = %{version}-%{release}
> not
> Requires: %{name} = %{version}-%{release}
> 
> - Directories %{_includedir}/sapi and %{_includedir}/tcti should be owned.
> Please change 
> %{_includedir}/sapi/*.h
> %{_includedir}/tcti/*.h
> into
> %{_includedir}/sapi
> %{_includedir}/tcti
> 
> 
> I am going to attach a patch proposal comprising all changes up to here to
> this BZ.
> 
> 
> 
> 
> - IMHO, the CFLAGS in upstream's *.pc.ins are bogus.
> They all contain
> Cflags: -I@includedir@/<subpkg>
> while I think they should contain
> Cflags: -I@includedir@
> 
> What actually is correct, depends upon which form of #include statements
> upstream expects consumers/users of these libs to use: 
> If they are expected to use "#include <someheader.h>"
> then the 1st form would be correct.
> If they are expected to use "#include <subpkg/someheader.h>"
> then the 2nd form would be correct.
> 
> In general, the 2nd form is better, because the 1st form is more likely to
> erroniously pickup bogus headers from the compiler's include path.
> 
> 
> Finally, one general (upstream-related) question:
> On which architectures/target-platform is this package useful on/applicable
> to?
> I guess, it's probably x86_64 + i686 only (or x86_64 only?), but not on
> others (arm, sparc, s3**, ppc, ...).

(In reply to yunying.sun from comment #16)
> (In reply to Ralf Corsepius from comment #13)
> > (In reply to yunying.sun from comment #12)
> > > Could you help to review again?
> > 
> > Here we go:
> > 
> > - Please append --disable-silent-rules to %configure
> > This enforces verbose building, because silent building is not helpful when
> > building packages in batches.
> > 
> > - Please disable-static libs.
> > Static libs are strongly discouraged in Fedora.
> > 
> > To achieve this, append --disable-static to %configure
> > and remove %{_libdir}/*.a from %files
> > 
> @Ralf, after disabling static library build, the other package tpm2-tools
> local rpm build fails due to not finding SAPI. Reason is that it depens on
> libsapi.a & libtcti-*.a which ought to be generated from tpm2-tss build.
> 
> What is the right way to solve this issue?
> 
> On wiki:
> https://fedoraproject.org/wiki/Packaging:
> Guidelines#Packaging_Static_Libraries, one solution is put static libs into
> a *-static subpackage, and use "BuildRequires: *-static" for packages which
> links to this static subpackage.
> 
> Is this the right way to go?
> 

Sorry, false alarm. I forgot to install tpm2-tss(devel).rpm before doing rpm local build.. Please ignore the problem in comment #16.

There are still *.la files in -devel. They must not be included.

Trusted Platform Module *could* be installed (and it did) on arm, ppc or any other platforms. So I do not see any reasons to restrict builds.

Please locate spec file to location anyone could wget it.

In addition: and src.rpm too.

(In reply to Dmitrij S. Kryzhevich from comment #18)
> There are still *.la files in -devel. They must not be included.
*.la removed from SPEC.
> 
> Trusted Platform Module *could* be installed (and it did) on arm, ppc or any
> other platforms. So I do not see any reasons to restrict builds.
ExclusiveArch removed, so that no restrict builds based on platforms/archs.

> 
> Please locate spec file to location anyone could wget it.
I've verified below 2 url works well for wget.
updated SPEC: https://raw.githubusercontent.com/yunyings/share/master/tpm2-tss.spec
updated SRPM: https://github.com/yunyings/share/raw/master/tpm2-tss-1.0-0.1.beta1.el7.src.rpm

Succeeded koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=15514060
Succeeded COPR build: https://copr.fedorainfracloud.org/coprs/yunyings/tpm2-tss/build/449820/

@Dmitrij, please help to double check the updated SPEC. Thanks a lot.

(In reply to yunying.sun from comment #20)
> (In reply to Dmitrij S. Kryzhevich from comment #18)
> > There are still *.la files in -devel. They must not be included.
> *.la removed from SPEC.
> > 
> > Trusted Platform Module *could* be installed (and it did) on arm, ppc or any
> > other platforms. So I do not see any reasons to restrict builds.
> ExclusiveArch removed, so that no restrict builds based on platforms/archs.
> 
Upstream maintainer Philip & Jimmy confirmed that tpm2-tss currently does not support big endian architecture, and it has been verified only against Intel x86 & x86_64.

So I updated SPEC again, to restrict builds to ix86 & x86_64 only by adding:
ExclusiveArch:  %{ix86} x86_64

SPEC: https://raw.githubusercontent.com/yunyings/share/master/tpm2-tss.spec
SRPMS: https://github.com/yunyings/share/raw/master/tpm2-tss-1.0-0.1.beta1.el7.src.rpm
Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=15539169
COPR build: https://copr.fedorainfracloud.org/coprs/yunyings/tpm2-tss/build/450601/

@Dmitrij, could you help to review it again? Thanks.

There is no way to get sources via URL in Source0 tag. Moreover, sources in src.rpm differs from upstream.

(In reply to Dmitrij S. Kryzhevich from comment #22)
> There is no way to get sources via URL in Source0 tag. Moreover, sources in
> src.rpm differs from upstream.

Thanks for reminding, Dmitrij.

When downloaded through web, the source package name(1.0-beta_1) in url is different with the downloaded tarball name(TPM2.0-TSS-1.0-beta_1).
While if download using wget, the downloaded package name is same with url name(1.0-beta_1).

I added a script to rename downloaded package name, just for the web download issue. There're also some other changes: updated ExcludeArch, use autosetup instead of setup, use "-n" to specify the uncompressed folder name during autosetup.

SPEC & SRPM has been updated:
SPEC: https://raw.githubusercontent.com/yunyings/share/master/tpm2-tss.spec
SRPMS: https://github.com/yunyings/share/raw/master/tpm2-tss-1.0-0.1.beta1.el7.src.rpm

Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=15597272
COPR build: https://copr.fedorainfracloud.org/coprs/yunyings/tpm2-tss/build/451936/

Please help to review again. Thanks.

I meant this:

$ md5sum tpm2-tss-1.0.tar.gz 1.0-beta_1.tar.gz 
2cdda79640f1e2e534ff897480190686  tpm2-tss-1.0.tar.gz
3d6df831cee731d95ef754f6599b3a6c  1.0-beta_1.tar.gz

tpm2-tss-1.0.tar.gz was gotten from prev src.rpm.
Now that's OK (anyway, diff see no changes in sources but still was a blocker).

Little issue: I see tabs and spaces in spec. I's better use one style.

And please refer documentation you used to mark license for sapi headers. That is not an issue from me, I just don't know whether it could be marked as BSD or not.

You used %autosetup so no epel6 builds, right?

(In reply to Dmitrij S. Kryzhevich from comment #24)
> I meant this:
> 
> $ md5sum tpm2-tss-1.0.tar.gz 1.0-beta_1.tar.gz 
> 2cdda79640f1e2e534ff897480190686  tpm2-tss-1.0.tar.gz
> 3d6df831cee731d95ef754f6599b3a6c  1.0-beta_1.tar.gz
> 
> tpm2-tss-1.0.tar.gz was gotten from prev src.rpm.
> Now that's OK (anyway, diff see no changes in sources but still was a
> blocker).
In previous SRPM, I downloaded source tarball from upstream, uncompressed, then change folder name manually(TPM2.0-TSS-1.0-beta_1.tar.gz -> tpm2-tss-1.0.tar.gz) and compress it again. Without manually name change, rpm failed to find the package with name %{name}-%{version}.tar.gz.

> 
> Little issue: I see tabs and spaces in spec. I's better use one style.
Corrected.

> 
> And please refer documentation you used to mark license for sapi headers.
> That is not an issue from me, I just don't know whether it could be marked
> as BSD or not.
Confirmed with upstream that BSD would be OK as well for sapi headers.

> 
> You used %autosetup so no epel6 builds, right?
We are trying to add this package for rawhide(latest Fedora development version) first, so EPEL7 is enough.

Updated SPEC & SRPM:
SPEC: https://raw.githubusercontent.com/yunyings/share/master/tpm2-tss.spec
SRPMS: https://github.com/yunyings/share/raw/master/tpm2-tss-1.0-0.1.beta1.el7.src.rpm

COPR build: https://copr.fedorainfracloud.org/coprs/yunyings/tpm2-tss/build/452011/

@Dmitrij, please help to review again. Really appreciate.

You don't need the silly rename-tarball_tss.sh script (which would need be changed with each release anyway).  Preferred mechanism is in https://fedoraproject.org/wiki/Packaging:SourceURL#Troublesome_URLs

For most github projects we use:

Source0:        https://github.com/01org/TPM2.0-TSS/archive/%{pkg_version}.tar.gz#/%{name}-%{version}.tar.gz

but you might want:

Source0:        https://github.com/01org/TPM2.0-TSS/archive/%{pkg_version}.tar.gz#/%{pkg_prefix}-%{pkg_version}.tar.gz

In any case, do not repack upstream tarballs unless you absolutely have to.

FWIW - %autosetup is present in EPEL6,  Bigger issue is older autoconf in EL6.

(In reply to Orion Poplawski from comment #26)
> You don't need the silly rename-tarball_tss.sh script (which would need be
> changed with each release anyway).  Preferred mechanism is in
> https://fedoraproject.org/wiki/Packaging:SourceURL#Troublesome_URLs
> 

@Orion, thanks for highlighting the SourceURL issue. I removed the script and updated Source0 URL. Could you help to review again? Thanks again.

Updated SPEC: https://raw.githubusercontent.com/yunyings/share/master/tpm2-tss.spec
Updated SRPMS: https://github.com/yunyings/share/raw/master/tpm2-tss-1.0-0.1.beta1.el7.src.rpm
Succeeded COPR build: https://copr.fedorainfracloud.org/coprs/yunyings/tpm2-tss/build/452354/

@Igor, Ralf, Dmitrij, Orion, could you kindly help to review this package again? Any comments would be appreciated. Thanks.

Fixed a typo in ExcludeArch list.

Updated SPEC: https://raw.githubusercontent.com/yunyings/share/master/tpm2-tss.spec
Updated SRPMS: https://github.com/yunyings/share/raw/master/tpm2-tss-1.0-0.1.beta1.el7.src.rpm

Succeeded Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=15817437
Succeeded COPR build: https://copr.fedorainfracloud.org/coprs/yunyings/tpm2-tss/build/458077/

Anyone could kindly help to review, and maybe approve if no more issues found?

Changelog needs to be updated every time you make changes:
https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Changelogs



The package failed to build on Fedora 24 (x86_64) as below:

libtool: compile:  g++ -DPACKAGE_NAME=\"tpm2.0-tss\" -DPACKAGE_TARNAME=\"tpm2-0-tss\" -DPACKAGE_VERSION=\"0.98\" "-DPACKAGE_STRING=\"tpm2.0-tss 0.98\"" -DPACKAGE_BUGREPORT=\"\" -DPACKAGE_URL=\"\" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 -DLT_OBJDIR=\".libs/\" -DHAVE_PTHREAD_PRIO_INHERIT=1 -DHAVE_PTHREAD=1 -DPACKAGE=\"tpm2-0-tss\" -DVERSION=\"0.98\" -I. -I./include -I./common -I./sysapi/include -I./include -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic -c common/sockets.cpp  -o common/.libs/tcti_libtcti_socket_la-sockets.o
./libtool: line 1763: g++: command not found
Makefile:5819: recipe for target 'common/tcti_libtcti_socket_la-sockets.lo' failed
make: *** [common/tcti_libtcti_socket_la-sockets.lo] Error 1
make: *** Waiting for unfinished jobs....
libtool: compile:  g++ -DPACKAGE_NAME=\"tpm2.0-tss\" -DPACKAGE_TARNAME=\"tpm2-0-tss\" -DPACKAGE_VERSION=\"0.98\" "-DPACKAGE_STRING=\"tpm2.0-tss 0.98\"" -DPACKAGE_BUGREPORT=\"\" -DPACKAGE_URL=\"\" -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 -DLT_OBJDIR=\".libs/\" -DHAVE_PTHREAD_PRIO_INHERIT=1 -DHAVE_PTHREAD=1 -DPACKAGE=\"tpm2-0-tss\" -DVERSION=\"0.98\" -I. -I./include -I./common -I./sysapi/include -I./include -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic -c tcti/tcti_socket.cpp  -o tcti/.libs/tcti_libtcti_socket_la-tcti_socket.o
./libtool: line 1763: g++: command not found
Makefile:5812: recipe for target 'tcti/tcti_libtcti_socket_la-tcti_socket.lo' failed
make: *** [tcti/tcti_libtcti_socket_la-tcti_socket.lo] Error 1

(In reply to lo1 from comment #30)
> Changelog needs to be updated every time you make changes:
> https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/
> Guidelines#Changelogs
> 
Thanks for the review Vicky. About change logs, on wiki it says "Every time you make changes, that is, whenever you increment the E-V-R of a package, add a changelog entry. ". My understanding is only when version and/or release is updated, changelog needed. 

In my case, the target version/release is the package's first release, 1.0-beta1, not changed. So I suppose it's better not to log every detailed changes which I made to fix issues found during review, otherwise the changelog would be long and full of minor details. Is it acceptable?

> 
> 
> The package failed to build on Fedora 24 (x86_64) as below:
> 
Your build failed due to not finding g++. But on koji build server and COPR, this package does compile successfully. I did local rpmbuild on my desktop with RHEL 7.3 Alpha(No Fedora 24 environment so far). Does Fedora 24 local build is mandatory for new package?

(In reply to yunying.sun from comment #31)
> (In reply to lo1 from comment #30)
> > Changelog needs to be updated every time you make changes:
> > https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/
> > Guidelines#Changelogs
> > 
> Thanks for the review Vicky. About change logs, on wiki it says "Every time
> you make changes, that is, whenever you increment the E-V-R of a package,
> add a changelog entry. ". My understanding is only when version and/or
> release is updated, changelog needed. 
> 
> In my case, the target version/release is the package's first release,
> 1.0-beta1, not changed. So I suppose it's better not to log every detailed
> changes which I made to fix issues found during review, otherwise the
> changelog would be long and full of minor details. Is it acceptable?
> 

You're right.  You can do this any number of times, until you actually build 1.0-0.1.beta1 in the buildsystem. By the way, the format of the changelog, in particular the version-release at the end is not the expected.   You can search for "%changelog:" in the link: https://fedoraproject.org/wiki/How_to_create_an_RPM_package#Introduction

> > 
> > 
> > The package failed to build on Fedora 24 (x86_64) as below:
> > 
> Your build failed due to not finding g++. But on koji build server and COPR,
> this package does compile successfully. I did local rpmbuild on my desktop
> with RHEL 7.3 Alpha(No Fedora 24 environment so far). Does Fedora 24 local
> build is mandatory for new package?

No, I dont think the Fedora 24 local build is mandatory.  Sorry, I totally ignored the koji and copr for some reasons.  To clarify, Koji and Copr build would be more appropriate and "clean" as my local one.

> No, I dont think the Fedora 24 local build is mandatory.  Sorry, I totally
> ignored the koji and copr for some reasons.  To clarify, Koji and Copr build
> would be more appropriate and "clean" as my local one.
Typo corrected:
To clarify, Koji and Copr build would be more appropriate and "clean" than my local one.

(In reply to lo1 from comment #32)
> (In reply to yunying.sun from comment #31)
> > (In reply to lo1 from comment #30)
> By the way, the format of the changelog,
> in particular the version-release at the end is not the expected.   You can
> search for "%changelog:" in the link:
> https://fedoraproject.org/wiki/How_to_create_an_RPM_package#Introduction
> 
Thanks for notice Vicky. I've updated SPEC to use V-R format in %changelog. 

Updated SPEC: https://raw.githubusercontent.com/yunyings/share/master/tpm2-tss.spec
Updated SRPMS: https://github.com/yunyings/share/raw/master/tpm2-tss-1.0-0.1.beta1.el7.src.rpm

Succeeded Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=15854449
Succeeded COPR build: https://copr.fedorainfracloud.org/coprs/yunyings/tpm2-tss/build/458998/

Any review comments are appreciated. Also could someone help to approve this review request if no more issues? Thanks in advance.

(In reply to yunying.sun from comment #31)
> (In reply to lo1 from comment #30)
> > Changelog needs to be updated every time you make changes:
> > https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/
> > Guidelines#Changelogs
> > 
> Thanks for the review Vicky. About change logs, on wiki it says "Every time
> you make changes, that is, whenever you increment the E-V-R of a package,
> add a changelog entry. ". My understanding is only when version and/or
> release is updated, changelog needed. 
> 
> In my case, the target version/release is the package's first release,
> 1.0-beta1, not changed. So I suppose it's better not to log every detailed
> changes which I made to fix issues found during review, otherwise the
> changelog would be long and full of minor details. Is it acceptable?
> 
> > 
> > 
> > The package failed to build on Fedora 24 (x86_64) as below:
> > 
> Your build failed due to not finding g++. But on koji build server and COPR,
> this package does compile successfully. I did local rpmbuild on my desktop
> with RHEL 7.3 Alpha(No Fedora 24 environment so far). Does Fedora 24 local
> build is mandatory for new package?
All BuildRequires must be specified. Even gcc/gcc-c++.

(In reply to Igor Gnatenko from comment #35)
> (In reply to yunying.sun from comment #31)
> > (In reply to lo1 from comment #30)
> All BuildRequires must be specified. Even gcc/gcc-c++.

@Igor, there're cpp source files in this package. I've added "BuildRequires: gcc", and there're no compile errors in koji & Copr build. Do I have to add "BuildRequires: gcc-c++" in this case?

(In reply to Igor Gnatenko from comment #35)
> (In reply to yunying.sun from comment #31)
> > (In reply to lo1 from comment #30)
> All BuildRequires must be specified. Even gcc/gcc-c++.

gcc package description says "gcc.x86_64 : Various compilers (C, C++, Objective-C, Java, ...)", does that mean gcc compile also works for c++ source files, so gcc-c++ is not necessarily needed?

(In reply to Igor Gnatenko from comment #35)
> (In reply to yunying.sun from comment #31)
> > (In reply to lo1 from comment #30)
> > > Changelog needs to be updated every time you make changes:
> > > https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/
> > > Guidelines#Changelogs
> > > 
> > Thanks for the review Vicky. About change logs, on wiki it says "Every time
> > you make changes, that is, whenever you increment the E-V-R of a package,
> > add a changelog entry. ". My understanding is only when version and/or
> > release is updated, changelog needed. 
> > 
> > In my case, the target version/release is the package's first release,
> > 1.0-beta1, not changed. So I suppose it's better not to log every detailed
> > changes which I made to fix issues found during review, otherwise the
> > changelog would be long and full of minor details. Is it acceptable?
> > 
> > > 
> > > 
> > > The package failed to build on Fedora 24 (x86_64) as below:
> > > 
> > Your build failed due to not finding g++. But on koji build server and COPR,
> > this package does compile successfully. I did local rpmbuild on my desktop
> > with RHEL 7.3 Alpha(No Fedora 24 environment so far). Does Fedora 24 local
> > build is mandatory for new package?
> All BuildRequires must be specified. Even gcc/gcc-c++.

Previusly, as I saw Yunying's koji and copr output built fine in the rawhide, I did a little digging and found that gcc and gcc-c++ are the exceptions.  The "exception" section in the link below stated that: 
https://fedoraproject.org/wiki/HOWTOFindMissingBuildRequires

Now, I'm wondering if that exception is a very recent rule, which wouldn't apply to an older release like Fed24.

(In reply to lo1 from comment #38)
> Previusly, as I saw Yunying's koji and copr output built fine in the
> rawhide, I did a little digging and found that gcc and gcc-c++ are the
> exceptions.  The "exception" section in the link below stated that: 
> https://fedoraproject.org/wiki/HOWTOFindMissingBuildRequires
> 
> Now, I'm wondering if that exception is a very recent rule, which wouldn't
> apply to an older release like Fed24.

That page is not part of the official packaging guidelines (not under Packaging:) and is not up to date.  For example, perl was recently pulled from the buildroot.

(In reply to Orion Poplawski from comment #39)
> (In reply to lo1 from comment #38)
> > Previusly, as I saw Yunying's koji and copr output built fine in the
> > rawhide, I did a little digging and found that gcc and gcc-c++ are the
> > exceptions.  The "exception" section in the link below stated that: 
> > https://fedoraproject.org/wiki/HOWTOFindMissingBuildRequires
> > 
> > Now, I'm wondering if that exception is a very recent rule, which wouldn't
> > apply to an older release like Fed24.
> 
> That page is not part of the official packaging guidelines (not under
> Packaging:) and is not up to date.  For example, perl was recently pulled
> from the buildroot.
Good to know.  Thanks! I have to develop the habit of checking the 'history' now!

Hi everyone, this is my first package, and after some reviews and updates, it has been quiet for some days. Could someone kindly help with further review, approve and sponsor? Thanks in advance.

Created attachment 1214569 [details]
License-file contained in upstream-tarball

Well, the license-file shipped with the sources contains some possible restrictions on the user's freedom of use…

Raising fedora-legal here to clarify, before starting the review…

I just added the TCGL to the Fedora license list. It is Free and GPL compatible, but only when applied to source code. When applied to documentation (specifically, standards docs), it is non-free. Please be sure that any standards documentation under the TCGL are not included in this package.

The license tag for this package should include TCGL and BSD. Lifting FE-Legal.

(In reply to Tom "spot" Callaway from comment #43)
> I just added the TCGL to the Fedora license list. It is Free and GPL
> compatible, but only when applied to source code. When applied to
> documentation (specifically, standards docs), it is non-free. Please be sure
> that any standards documentation under the TCGL are not included in this
> package.
Thanks for the license review.

Confirmed that no standards documents are included in this package. Only 2 header files in this package are under TCGL license: 
include/sapi/implementation.h
include/sapi/tpmb.h

> 
> The license tag for this package should include TCGL and BSD. Lifting
> FE-Legal.
Done. SPEC updated, license changed to BSD and TCGL, together with related comment.

Updated SPEC: https://raw.githubusercontent.com/yunyings/share/master/tpm2-tss.spec
Updated SRPMS: https://github.com/yunyings/share/raw/master/tpm2-tss-1.0-0.1.beta1.el7.src.rpm

Succeeded Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=16276958
Succeeded COPR build: https://copr.fedorainfracloud.org/coprs/yunyings/tpm2-tss/build/472424/

Please follow the official process to get sponsored as a packager:
https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group

Upstream has released 1.0 at Nov 3 2016: https://github.com/01org/TPM2.0-TSS/releases

I updated this package to 1.0 upstream as well, to make sure this package is up to date.

Updated SPEC: https://raw.githubusercontent.com/yunyings/share/master/tpm2-tss.spec
Updated SRPM: https://github.com/yunyings/share/blob/master/tpm2-tss-1.0-1.el7.src.rpm?raw=true

Succeeded Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=16456652
Succeeded COPR build: https://copr.fedorainfracloud.org/coprs/yunyings/tpm2-tss/build/476765/

(In reply to Raphael Groner from comment #45)
> Please follow the official process to get sponsored as a packager:
> https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group

I've done some reviews, see comments in bug 1385331 and bug 1366047. Will try to do more reviews in future.

More reviews are done on 1395522, 1394789, 1399648, 1398922.

(In reply to Björn "besser82" Esser from comment #42)
> Created attachment 1214569 [details]
> License-file contained in upstream-tarball
> 
> Well, the license-file shipped with the sources contains some possible
> restrictions on the user's freedom of use…
> 
> Raising fedora-legal here to clarify, before starting the review…

Hi Bjorn, since the license issue has been fixed, could you help to do a formal review of this package? I'd be happy to see more comments, or approval for sure.

(running fedora-review on this package myself)

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

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed


Issues:
=======
- All build dependencies are listed in BuildRequires, except for any that
  are listed in the exceptions section of Packaging Guidelines.
  Note: These BR are not needed: gcc gcc-c++
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2


===== MUST items =====

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

Generic:
[ ]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[ ]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "BSD (2 clause)", "Unknown or generated". 23 files have unknown
     license. Detailed output of licensecheck in
     /home/test/yunyings/rpmreview/1369708_tpm2-tss/review-
     tpm2-tss/licensecheck.txt
[ ]: License file installed when any subpackage combination is installed.
[ ]: If the package is under multiple licenses, the licensing breakdown
     must be documented in the spec.
[ ]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr, /usr/include,
     /usr/lib64, /usr/share/licenses, /usr/lib64/pkgconfig, /usr/share,
     /usr/sbin, /usr/share/doc
[ ]: %build honors applicable compiler flags or justifies otherwise.
[ ]: Package contains no bundled libraries without FPC exception.
[ ]: Changelog in prescribed format.
[ ]: Sources contain only permissible code or content.
[ ]: Package contains desktop file if it is a GUI application.
[ ]: Development files must be in a -devel package
[ ]: Package uses nothing in %doc for runtime.
[ ]: Package consistently uses macros (instead of hard-coded directory
     names).
[ ]: Package is named according to the Package Naming Guidelines.
[ ]: Package does not generate any conflict.
[ ]: Package obeys FHS, except libexecdir and /usr/target.
[ ]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[ ]: Requires correct, justified where necessary.
[ ]: Spec file is legible and written in American English.
[ ]: Package contains systemd file(s) if in need.
[ ]: Useful -debuginfo package or justification otherwise.
[ ]: Package is not known to require an ExcludeArch tag.
[ ]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 40960 bytes in 2 files.
[ ]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: 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 %license.
[x]: Package requires other packages for directories it uses.
[x]: Package does not own files or directories owned by other packages.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[ ]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[ ]: Final provides and requires are sane (see attachments).
[ ]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in tpm2
     -tss-devel
[ ]: Package functions as described.
[ ]: Latest version is packaged.
[ ]: Package does not include license text files separate from upstream.
[ ]: Scriptlets must be sane, if used.
[ ]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[ ]: Package should compile and build into binary rpms on all supported
     architectures.
[ ]: %check is present and all tests pass.
[ ]: Packages should try to preserve timestamps of original installed
     files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: The placement of pkgconfig(.pc) files are correct.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on debuginfo package(s).
     Note: There are rpmlint messages (see attachment).
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
[x]: Package should not use obsolete m4 macros
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: tpm2-tss-1.0-1.el7.centos.x86_64.rpm
          tpm2-tss-devel-1.0-1.el7.centos.x86_64.rpm
          tpm2-tss-1.0-1.el7.centos.src.rpm
tpm2-tss.x86_64: W: incoherent-version-in-changelog 1.0-1 ['1.0-1.el7.centos', '1.0-1.centos']
tpm2-tss.x86_64: W: invalid-license TCGL
tpm2-tss.x86_64: W: no-manual-page-for-binary resourcemgr
tpm2-tss-devel.x86_64: W: invalid-license TCGL
tpm2-tss-devel.x86_64: W: no-documentation
tpm2-tss.src: W: invalid-license TCGL
tpm2-tss.src: W: strange-permission tpm2-tss.spec 0775L
tpm2-tss.src:6: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 6)
3 packages and 0 specfiles checked; 0 errors, 8 warnings.




Rpmlint (debuginfo)
-------------------
Checking: tpm2-tss-debuginfo-1.0-1.el7.centos.x86_64.rpm
tpm2-tss-debuginfo.x86_64: W: invalid-license TCGL
1 packages and 0 specfiles checked; 0 errors, 1 warnings.





Rpmlint (installed packages)
----------------------------
tpm2-tss.x86_64: W: incoherent-version-in-changelog 1.0-1 ['1.0-1.el7.centos', '1.0-1.centos']
tpm2-tss.x86_64: W: invalid-license TCGL
tpm2-tss.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libtcti-socket.so.0.0.0 /lib64/libstdc++.so.6
tpm2-tss.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libtcti-socket.so.0.0.0 /lib64/libm.so.6
tpm2-tss.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libtcti-socket.so.0.0.0 /lib64/libgcc_s.so.1
tpm2-tss.x86_64: W: no-manual-page-for-binary resourcemgr
tpm2-tss-debuginfo.x86_64: W: invalid-license TCGL
tpm2-tss-debuginfo.x86_64: W: only-non-binary-in-usr-lib
tpm2-tss-devel.x86_64: W: invalid-license TCGL
tpm2-tss-devel.x86_64: W: no-documentation
3 packages and 0 specfiles checked; 0 errors, 10 warnings.



Requires
--------
tpm2-tss (rpmlib, GLIBC filtered):
    /sbin/ldconfig
    libc.so.6()(64bit)
    libgcc_s.so.1()(64bit)
    libm.so.6()(64bit)
    libpthread.so.0()(64bit)
    libsapi.so.0()(64bit)
    libstdc++.so.6()(64bit)
    libtcti-device.so.0()(64bit)
    libtcti-socket.so.0()(64bit)
    rtld(GNU_HASH)

tpm2-tss-devel (rpmlib, GLIBC filtered):
    /usr/bin/pkg-config
    libsapi.so.0()(64bit)
    libtcti-device.so.0()(64bit)
    libtcti-socket.so.0()(64bit)
    pkgconfig(sapi)
    tpm2-tss(x86-64)



Provides
--------
tpm2-tss:
    libsapi.so.0()(64bit)
    libtcti-device.so.0()(64bit)
    libtcti-socket.so.0()(64bit)
    tpm2-tss
    tpm2-tss(x86-64)

tpm2-tss-devel:
    pkgconfig(sapi)
    pkgconfig(tcti-device)
    pkgconfig(tcti-socket)
    tpm2-tss-devel
    tpm2-tss-devel(x86-64)



Source checksums
----------------
https://github.com/01org/TPM2.0-TSS/archive/1.0.tar.gz#/TPM2.0-TSS-1.0.tar.gz :
  CHECKSUM(SHA256) this package     : b5697cfe7f4fd44d6ae1ec03cddb6b44d5cf5cd13e134c7238049551d1615488
  CHECKSUM(SHA256) upstream package : b5697cfe7f4fd44d6ae1ec03cddb6b44d5cf5cd13e134c7238049551d1615488


Generated by fedora-review 0.5.3 (bcf15e3) last change: 2015-05-04
Command line :/usr/bin/fedora-review -n tpm2-tss
Buildroot used: epel-7-x86_64
Active plugins: Generic, Shell-api, C/C++
Disabled plugins: Java, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP, Ruby
Disabled flags: EXARCH, DISTTAG, EPEL5, BATCH, EPEL6

Regarding some of the rpmlint warnings:

> tpm2-tss.src: W: invalid-license TCGL
TCGL license has been added to the Fedora license list by Tom, see comment 43.

tpm2-tss.src: W: strange-permission tpm2-tss.spec 0775L
> fixed.

tpm2-tss.src:6: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 6)
> fixed.

Updated SPEC: https://raw.githubusercontent.com/yunyings/share/master/tpm2-tss.spec
Updated SRPM: https://github.com/yunyings/share/blob/master/tpm2-tss-1.0-1.el7.src.rpm?raw=true

Succeeded Koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=16704153
Succeeded COPR build: https://copr.fedorainfracloud.org/coprs/yunyings/tpm2-tss/build/483970/

Björn, are you going to continue with the review here?

Isn't this request a duplicate of https://admin.fedoraproject.org/pkgdb/package/rpms/tss2/ ?

(In reply to Dan Horák from comment #53)
> Isn't this request a duplicate of
> https://admin.fedoraproject.org/pkgdb/package/rpms/tss2/ ?

Maybe... but I think it is a separate implementation of similar functionality?  

Vicky and Tomas the the review on tss2 and Yunying was also commented on it.  They're all on CC for this one as well, so I think we can get that cleared up soon.

(In reply to Josh Boyer from comment #54)
> (In reply to Dan Horák from comment #53)
> > Isn't this request a duplicate of
> > https://admin.fedoraproject.org/pkgdb/package/rpms/tss2/ ?
> 
> Maybe... but I think it is a separate implementation of similar
> functionality?  
It seems true to me, that it's separate user space implementations of TCG's TSS 
for TPM2.0. The functionalities and usage should be the same mostly, but implementations are different.

Intel's implementation contains 2 tools, the TPM2.0-TSS and tpm2.0-tools, while IBM's solution is all in one package.

@Jimmy, please do correct me if I misunderstood.

> 
> Vicky and Tomas the the review on tss2 and Yunying was also commented on it.
> They're all on CC for this one as well, so I think we can get that cleared
> up soon.
@Josh, What do you mean the "cleaned up"? Will this package have to be rejected for Fedora due to existing approved tss2, or co-existing is allowed?

(In reply to Yunying Sun from comment #55)
> (In reply to Josh Boyer from comment #54)
> > (In reply to Dan Horák from comment #53)
> > > Isn't this request a duplicate of
> > > https://admin.fedoraproject.org/pkgdb/package/rpms/tss2/ ?
> > 
> > Maybe... but I think it is a separate implementation of similar
> > functionality?  
> It seems true to me, that it's separate user space implementations of TCG's
> TSS 
> for TPM2.0. The functionalities and usage should be the same mostly, but
> implementations are different.
> 
> Intel's implementation contains 2 tools, the TPM2.0-TSS and tpm2.0-tools,
> while IBM's solution is all in one package.
> 
> @Jimmy, please do correct me if I misunderstood.
> 
> > 
> > Vicky and Tomas the the review on tss2 and Yunying was also commented on it.
> > They're all on CC for this one as well, so I think we can get that cleared
> > up soon.
> @Josh, What do you mean the "cleaned up"? Will this package have to be
> rejected for Fedora due to existing approved tss2, or co-existing is allowed?

Yes, they can co-exist in Fedora, but we need to be careful about things like file conflicts (https://fedoraproject.org/wiki/Packaging:Conflicts). But I was mainly curious about the relationship between these two.

Looking to the future and RHEL, I noticed when taking a look autoconf-archive and libcmocka are only available in EPEL. The autoconf-archive was just using the pthread macro, so should be easy enough to deal with. Cmocka is used in the test application.

(In reply to Yunying Sun from comment #55)
> (In reply to Josh Boyer from comment #54)
> > (In reply to Dan Horák from comment #53)
> > > Isn't this request a duplicate of
> > > https://admin.fedoraproject.org/pkgdb/package/rpms/tss2/ ?
> > 
> > Maybe... but I think it is a separate implementation of similar
> > functionality?  
> It seems true to me, that it's separate user space implementations of TCG's
> TSS 
> for TPM2.0. The functionalities and usage should be the same mostly, but
> implementations are different.
> 
> Intel's implementation contains 2 tools, the TPM2.0-TSS and tpm2.0-tools,
> while IBM's solution is all in one package.
> 
> @Jimmy, please do correct me if I misunderstood.

Yunying is right. Intel's implementation of TSS2 is strictly following the TCG TSS2 SAPI & TCTI spec and is the reference implementation of the spec.

No reaction from Björn in the last week, I'll take over the review.

formal review is here, see the notes explaining OK* and BAD statuses below:

OK      source files match upstream:
            c610fa5273909394fa54174afcd7541a5c87d16b  TPM2.0-TSS-1.0.tar.gz
OK      package meets naming and versioning guidelines.
OK*     specfile is properly named, is cleanly written and uses macros consistently.
OK      dist tag is present.
OK      license field matches the actual license.
OK      license is open source-compatible. License text included in package.
OK      latest version is being packaged.
OK*     BuildRequires are proper.
OK      compiler flags are appropriate.
OK      package builds in mock (Rawhide/x86_64).
OK      debuginfo package looks complete.
OK*     rpmlint is silent.
OK      final provides and requires look sane.
N/A*    %check is present and all tests pass.
OK      shared libraries are added to the regular linker search paths.
OK      owns the directories it creates.
OK      doesn't own any directories it shouldn't.
OK      no duplicates in %files.
OK      file permissions are appropriate.
OK      correct scriptlets present.
OK      code, not content.
OK      documentation is small, so no -docs subpackage is necessary.
OK      %docs are not necessary for the proper functioning of the package.
OK      headers in devel.
OK      pkgconfig files in devel.
OK      no libtool .la droppings.
OK      not a GUI app.

- the %pkg_version macro is redundant, it contains the same value as %version
- instead of ExcludeArch, that's currently incomplete (doesn't include eg. mips or s390) "ExclusiveArch: %{ix86} x86_64"
  should be used, exactly per the reason stated in comment above the tag
- it's useful to write entries for directories with trailing slash, eg. %{_includedir}/sapi/ - makes visible to the reader it's a directory
- also useful is to be more specific (less wildcards) in entries like %{_libdir}/*.so.* - better control about what changed in the upstream project
- during the review process please increase the %release tag value for every revision of the package published, it allows to see what changed
- autoconf and automake can be dropped from BuildRequires (libtool brings them in), similar for pkgconfig (pkgconfig(cmocka) will bring it in)
- rpmlint complains a bit, but it can be ignored
tpm2-tss.x86_64: W: invalid-license TCGL
tpm2-tss.x86_64: W: no-manual-page-for-binary resourcemgr
tpm2-tss-devel.x86_64: W: invalid-license TCGL
tpm2-tss-devel.x86_64: W: only-non-binary-in-usr-lib
tpm2-tss-devel.x86_64: W: no-documentation
tpm2-tss.src: W: invalid-license TCGL
tpm2-tss-debuginfo.x86_64: W: invalid-license TCGL
4 packages and 0 specfiles checked; 0 errors, 7 warnings.
- a test directory is present in the source archive, can it be used (with --enable-unit)? If it isn't used, then pkgconfig(cmocka) can be removed from BR I suppose.

So almost good, but please answer my question (and/or prepare a new iteration) before I'll approve the package.

(In reply to Dan Horák from comment #60)
> formal review is here, see the notes explaining OK* and BAD statuses below:
> ...
> So almost good, but please answer my question (and/or prepare a new
> iteration) before I'll approve the package.

Thanks for the formal review and comments, Dan. I've updated SPEC & SRPM after fixing most issues you listed.

Updated SPEC: https://raw.githubusercontent.com/yunyings/share/master/tpm2-tss.spec
Updated SRPM: https://github.com/yunyings/share/blob/master/tpm2-tss-1.0-2.el7.src.rpm?raw=true
COPR build(not yet finished): https://copr.fedorainfracloud.org/coprs/yunyings/tpm2-tss/build/487392/

> - a test directory is present in the source archive, can it be used (with
> --enable-unit)? If it isn't used, then pkgconfig(cmocka) can be removed from
> BR I suppose.
Confirmed with upstream developer(Gang Wei) that test source code is not suggested to add into distro package, because "the software should be well tested before a formal upstream releasing, and the unit test code are just intended to help developer addressing wrong changes to the code." So no change for this.

Questions left:
1. > N/A*    %check is present and all tests pass.
Is the %check section a MUST?

2. koji build always fail with certification failure(re-fetched cert through fedora-packager-setup for couple of times but error remains same), and COPR build is still pending after being submitted for over 2 hours. Is there something wrong known with Koji & COPR build servers today?

(In reply to Yunying Sun from comment #61)
> (In reply to Dan Horák from comment #60)
> > formal review is here, see the notes explaining OK* and BAD statuses below:
> > ...
> > So almost good, but please answer my question (and/or prepare a new
> > iteration) before I'll approve the package.
> 
> Thanks for the formal review and comments, Dan. I've updated SPEC & SRPM
> after fixing most issues you listed.
> 
> Updated SPEC:
> https://raw.githubusercontent.com/yunyings/share/master/tpm2-tss.spec
> Updated SRPM:
> https://github.com/yunyings/share/blob/master/tpm2-tss-1.0-2.el7.src.
> rpm?raw=true
> COPR build(not yet finished):
> https://copr.fedorainfracloud.org/coprs/yunyings/tpm2-tss/build/487392/

thanks, will check them later today

> 
> > - a test directory is present in the source archive, can it be used (with
> > --enable-unit)? If it isn't used, then pkgconfig(cmocka) can be removed from
> > BR I suppose.
> Confirmed with upstream developer(Gang Wei) that test source code is not
> suggested to add into distro package, because "the software should be well
> tested before a formal upstream releasing, and the unit test code are just
> intended to help developer addressing wrong changes to the code." So no
> change for this.
> 
> Questions left:
> 1. > N/A*    %check is present and all tests pass.
> Is the %check section a MUST?

it's a SHOULD (https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Test_Suites), I would say if it's technically possible to run the test-suite, then it makes sense to run even during the build, it can be a slightly different environment than upstream uses (eg. stricter compiler flags) or it can test all architectures (not this case)
 
> 2. koji build always fail with certification failure(re-fetched cert through
> fedora-packager-setup for couple of times but error remains same), and COPR
> build is still pending after being submitted for over 2 hours. Is there
> something wrong known with Koji & COPR build servers today?

koji switched to kerberos based authentication today, please see https://lists.fedoraproject.org/archives/list/devel-announce@lists.fedoraproject.org/message/JK267PSDD53I2KGONDLFA5D4JWYXKZTQ/ for details

> > Questions left:
> > 1. > N/A*    %check is present and all tests pass.
> > Is the %check section a MUST?
> 
> it's a SHOULD
> (https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/
> Guidelines#Test_Suites), I would say if it's technically possible to run the
> test-suite, then it makes sense to run even during the build, it can be a
> slightly different environment than upstream uses (eg. stricter compiler
> flags) or it can test all architectures (not this case)
While in case of this package, Wei Gang suggested "unit tests should not be added into the build process of distro package", mainly due to:
1. Unit test code in upstream TPM2.0-TSS is not well organized yet, they are still working to add more formal unit test cases and make unit tests runnable during build process.
2. Currently, the unit test code is only intended to help developer addressing wrong changes to the code, and the formal released upstream code should have already been tested well.

So I'd prefer not adding %check this time, if that's ok from you. Possibly I can add it later when unit test has been verified to be working well during build process for upstream.

> > 2. koji build always fail with certification failure(re-fetched cert through
> > fedora-packager-setup for couple of times but error remains same), and COPR
> > build is still pending after being submitted for over 2 hours. Is there
> > something wrong known with Koji & COPR build servers today?
> 
> koji switched to kerberos based authentication today, please see
> https://lists.fedoraproject.org/archives/list/devel-announce@lists.
> fedoraproject.org/message/JK267PSDD53I2KGONDLFA5D4JWYXKZTQ/ for details
Thanks for the info. Just managed to start koji build and it succeeds:
https://koji.fedoraproject.org/koji/taskinfo?taskID=16862199

The COPR build also completed successfully:
https://copr.fedorainfracloud.org/coprs/yunyings/tpm2-tss/build/487392/

Please help to review again. Thanks a lot.

I'm fine with skipping the test-suite for now, all my other comments were accepted (or explained), this package is APPROVED. I'm going to you into the "packager" group, please follow with the next step in package inclusion process. If you have any additional question, don't hesitate to ask.

Thanks Dan, really appreciate the approval and sponsoring. 

I've created the new package request. "fedpkg clone" currently fails with error "Could not read from remote repository". Will try again once package request is approved.

Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/tpm2-tss

tpm2-tss-1.0-2.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2016-a49b0a42ea

tpm2-tss-1.0-2.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-22cc02f80e

SPEC & source tarball have been imported to SCM:
https://src.fedoraproject.org/cgit/rpms/tpm2-tss.git

Build completed for master/f25/epel7:
https://koji.fedoraproject.org/koji/packageinfo?packageID=23432

Command line "fedpkg update" failed:
[test@NUC-UEFI tpm2-tss]$ fedpkg update
Error: no such option: --bodhi-url
Could not execute update: Could not generate update request: Command 'bodhi --bodhi-url https://bodhi.fedoraproject.org/ updates new --file bodhi.template --user yunyings tpm2-tss-1.0-2.fc25' returned non-zero exit status 2

Seems known issue: https://bugzilla.redhat.com/show_bug.cgi?id=1404422

Just created 2 update requests(for f25 & epel7 each) through Bodhi web:
https://bodhi.fedoraproject.org/updates/?packages=tpm2-tss

tpm2-tss-1.0-2.el7 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-22cc02f80e

tpm2-tss-1.0-2.fc25 has been pushed to the Fedora 25 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-a49b0a42ea

It has been more than 4 days since the updates for this package been pushed to f25 & epel7 testing, but no update till now:
https://bodhi.fedoraproject.org/updates/FEDORA-2016-a49b0a42ea
https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-22cc02f80e

I left below options checked as default when submitting update request on Bodhi:
* Close bugs on stable?
* Auto-request stable?

Anything else I need to do to push it to stable?

Without karma, a Fedora update must stay in testing for seven days.  An EPEL update must stay in testing for fourteen days.

Of course, if you can get three positive test results on any update then it will go to stable automatically as long as you left "auto-request stable" enabled.

(In reply to Jason Tibbitts from comment #73)
> Without karma, a Fedora update must stay in testing for seven days.  An EPEL
> update must stay in testing for fourteen days.
> 
> Of course, if you can get three positive test results on any update then it
> will go to stable automatically as long as you left "auto-request stable"
> enabled.

Thanks for the information, Jason. 

I noticed below update being pushed to stable after 3 days waiting in testing, without karma:
https://bodhi.fedoraproject.org/updates/FEDORA-2016-20df2ce477
That's in Oct. Has the wait days been extended to 7 days recently?

(In reply to Yunying Sun from comment #74)
> That's in Oct. Has the wait days been extended to 7 days recently?
Yes. The time to wait changes after a release. It is 3 days (+rel-eng delays) during prerelease-phases and 7 days (+rel-eng delays) in post-release phases.

(In reply to Ralf Corsepius from comment #75)
> Yes. The time to wait changes after a release. It is 3 days (+rel-eng
> delays) during prerelease-phases and 7 days (+rel-eng delays) in
> post-release phases.
Thanks Ralf. Just saw related policies minutes ago at https://fedoraproject.org/wiki/Updates_Policy . Clear now.

tpm2-tss-1.0-2.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.

tpm2-tss-1.0-2.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.

tpm2-tss-1.0-2.el7 has been pushed to the Fedora EPEL 7 stable repository. If problems still persist, please make note of it in this bug report.