Bug 862160

Created attachment 620749 [details]
build.log with errors

I am not a member of the packager group, so this is just informal.

I was not able to build the package using fedora-review.

objects/tool_object.cpp: In member function 'bool ToolObject::startProcess(QStringList)':
objects/tool_object.cpp:391:26: error: 'usleep' was not declared in this scope
objects/tool_object.cpp: In member function 'void ToolObject::stopProcess()':
objects/tool_object.cpp:474:42: error: 'usleep' was not declared in this scope

The full build.log is attached.

Hi Sebastian,

Thanks for taking a look (and for the tip about fedora-review, trying that out now).

I've uploaded a new http://oss.sgi.com/~nathans/valkyrie-2.0.0-2.el6_3.src.rpm  which should fix this build issue.  I'll confirm with fedora-review once thats run (looks like mock will be awhile, downloading a build root atm).

cheers.

--
Nathan

Is this package intended for fedora and epel, or epel only? I have no centos 6 machine at hand only a centos 5 box.

Fedora and EPEL.  Same spec file should work everywhere though (I hope).

Works for me (f16) with the following rpmlint output:
rpmlint -v /home/tnorth/rpmbuild/SRPMS/valkyrie-2.0.0-2.fc16.src.rpm /home/tnorth/rpmbuild/RPMS/x86_64/valkyrie-2.0.0-2.fc16.x86_64.rpm /home/tnorth/rpmbuild/RPMS/x86_64/valkyrie-debuginfo-2.0.0-2.fc16.x86_64.rpm
valkyrie.src: I: checking
valkyrie.src: I: checking-url http://www.valgrind.org/ (timeout 10 seconds)
valkyrie.src: I: checking-url http://www.valgrind.org/downloads/valkyrie-2.0.0.tar.bz2 (timeout 10 seconds)
valkyrie.x86_64: I: checking
valkyrie.x86_64: I: checking-url http://www.valgrind.org/ (timeout 10 seconds)
valkyrie.x86_64: E: incorrect-fsf-address /usr/share/doc/valkyrie-2.0.0/COPYING
valkyrie.x86_64: W: no-manual-page-for-binary valkyrie
valkyrie.x86_64: W: install-file-in-docs /usr/share/doc/valkyrie-2.0.0/INSTALL
valkyrie-debuginfo.x86_64: I: checking
valkyrie-debuginfo.x86_64: I: checking-url http://www.valgrind.org/ (timeout 10 seconds)
3 packages and 0 specfiles checked; 1 errors, 2 warnings.

Looks like the FSF address needs fixing.

Does still not work for me (f17). I even tried a koji scratch build for rawhide, because I wanted to be sure that it is not my system. See http://koji.fedoraproject.org/koji/taskinfo?taskID=4560328

The build error is now:
utils/vk_config.cpp: In static member function 'static const QString& VkCfg::tmpDir()':
utils/vk_config.cpp:125:25: error: 'getuid' was not declared in this scope

It builds on f16, see: 
http://koji.fedoraproject.org/koji/taskinfo?taskID=4560328


Regards

Sebastian

(In reply to comment #5)
> valkyrie.x86_64: E: incorrect-fsf-address
> ...
> Looks like the FSF address needs fixing.

Doesn't need to be fixed at all. All we have to do is to inform the upstream people about that.

I've uploaded a new http://oss.sgi.com/~nathans/valkyrie-2.0.0-3.el6_3.src.rpm  which should fix this new build issue.

Will discuss the FSF address with upstream (as well as asking re a man page).

thanks!

Created attachment 622075 [details]
patch to include unistd.h in vk_utils.cpp

It still did not build for me. I have created a patch that fixes the last build error for me.

Thanks Sebastian - producing a patched spec now - could you let me know which symbol was missing there?  (will need to mention it to upstream)

Guess its one/more of these...?

                 U exit
                 U fflush
                 U free
                 U getenv
                 U getpid
                 U malloc
                 U mkstemp
                 U stderr
                 U stdout
                 U strcmp
                 U strcpy
                 U strlen

thanks.

It was getpid.

Error message was:

utils/vk_utils.cpp: In function 'void vkPrint(const char*, ...)':
utils/vk_utils.cpp:54:104: error: 'getpid' was not declared in this scope
utils/vk_utils.cpp: In function 'void vkPrintErr(const char*, ...)':
utils/vk_utils.cpp:68:104: error: 'getpid' was not declared in this scope
utils/vk_utils.cpp: In function 'void vkDebug(const char*, ...)':
utils/vk_utils.cpp:83:104: error: 'getpid' was not declared in this scope

Thanks Sebastian - I've uploaded http://oss.sgi.com/~nathans/valkyrie-2.0.0-4.el6_3.src.rpm (and spec file in same location) which include your patch.

You are intending to package this for EPEL5 right? If so, some issues are irrelevant.

There is a COPYING file which contains the license. It should be listed via %doc. The INSTALL file does not look like relevant documentation, it can be removed.

I think, the license is GPLv2+.


fedora-review produces the following issues:

Issues:
[!]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: defattr(....) present in %files section. This is OK if packaging
     for EPEL5. Otherwise not needed
See: http://fedoraproject.org/wiki/Packaging/Guidelines#FilePermissions
[!]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf is only needed if supporting EPEL5
See: None
[!]: MUST If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %doc.
See: http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text
[!]: MUST License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. No licenses
     found. Please check the source files for licenses manually.
See: http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#ValidLicenseShortNames

There are some SHOULD issues too:

[!]: SHOULD Buildroot is not present
     Note: Invalid buildroot found:
     %{_tmppath}/%{valkyrie}-%{release}-root-%(%{__id_u} -n) 
[!]: SHOULD Package has no %clean section with rm -rf %{buildroot} (or 
     $RPM_BUILD_ROOT)
     Note: Clean is needed only if supporting EPEL5
[!]: SHOULD Spec use %global instead of %define.
     Note: %define valkyrie %{name}-%{version}

(In reply to comment #13)
> I think, the license is GPLv2+.

The COPYING file says GPLv2, and the source file headers doesn't contain the "newer versions" clause. That's why it remains as GPLv2.

Sorry, my fault. I read the part of the COPYING about later versions again. Mario is right, it is GPLv2.

I've uploaded http://oss.sgi.com/~nathans/valkyrie-2.0.0-5.el6_3.src.rpm (and spec file in same location) which addresses the remaining issues, I think.
Going through the issues/questions listed in c13...

| You are intending to package this for EPEL5 right?

Yes.

| There is a COPYING file which contains the license. It should be listed via %doc.

*nod* - done.

| The INSTALL file does not look like relevant documentation, it can be removed.

I read through it again, I think there's some handy tips in there for users, so I left it for now (unless someone feels strongly that it should go?).

| I think, the license is GPLv2+.

As you guys discussed, I think its OK as is too (no +).

| fedora-review produces the following issues:
| Issues:
| [!]: MUST Each %files section contains %defattr if rpm < 4.4
|      Note: defattr(....) present in %files section. This is OK if packaging
|      for EPEL5. Otherwise not needed

Yep, keen to support EPEL5 too.

| See: http://fedoraproject.org/wiki/Packaging/Guidelines#FilePermissions
| [!]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
|      beginning of %install.
|      Note: rm -rf is only needed if supporting EPEL5

As above.

| [!]: MUST If (and only if) the source package includes the text of the
|      license(s) in its own file, then that file, containing the text of the
|      license(s) for the package is included in %doc.

Fixed.

| [!]: MUST License field in the package spec file matches the actual license.
|      Note: Checking patched sources after %prep for licenses. No licenses
|      found. Please check the source files for licenses manually.

COPYING is in %doc now, and License field looks correct as discussed.

| [!]: SHOULD Buildroot is not present
|      Note: Invalid buildroot found:
|      %{_tmppath}/%{valkyrie}-%{release}-root-%(%{__id_u} -n) 

It is present, and thats because we're going for EPEL5 support.
Not clear why it thinks this is invalid, perhaps its use of %{valkyrie}?
Certainly appears to work correctly, and have seen this general pattern
used in a number of other spec files.  Hmm, bit odd.

| [!]: SHOULD Package has no %clean section with rm -rf %{buildroot} (or 
|      $RPM_BUILD_ROOT)
|      Note: Clean is needed only if supporting EPEL5

Which we will (support EPEL5), so this is fine.

| [!]: SHOULD Spec use %global instead of %define.
|      Note: %define valkyrie %{name}-%{version}

Fixed.

I made a complete review now, based on valkyrie-2.0.0-5.el6_3.src.rpm.

There are two remaining issues, but I am not 100% sure about them.

First: Package uses nothing in %doc for runtime. You moved the html documentation in %doc. It is shown at runtime when you click on Help -> Contents. I think this is not really using it at runtime. If these html files are missing, the Online Documentation will be empty.

Second: %build honors applicable compiler flags or justifies otherwise. I do not see something like CFLAGS=... in your spec file and I am not familiar with qmake Makefile generation, environment CFLAGS set during build. If the package honors the compiler flags, tell me how it works :-).


I also do not know how to check: Packages should try to preserve timestamps of original installed files. It is only and I marked it with Not evaluated. As above, I would be thankful if someone could enlighten me howto check this.

I generated a koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=4575518

The full fedora-review output with manual checks:


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

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



==== C/C++ ====
[x]: MUST Header files in -devel subpackage, if present.
[x]: MUST Package does not contain any libtool archives (.la)
[x]: MUST Package does not contain kernel modules.
[x]: MUST Package contains no static executables.
[x]: MUST Rpath absent or only used for internal libs.


==== Generic ====
[x]: EXTRA Rpmlint is run on all installed packages.
     Note: No rpmlint messages.
[x]: MUST Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: MUST Package successfully compiles and builds into binary rpms on at
     least one supported primary architecture.
[ ]: MUST %build honors applicable compiler flags or justifies otherwise.
[x]: MUST All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[x]: MUST Package contains no bundled libraries.
[x]: MUST Changelog in prescribed format.
[x]: MUST Sources contain only permissible code or content.
[!]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: defattr(....) present in %files section. This is OK if packaging
     for EPEL5. Otherwise not needed
[x]: MUST Macros in Summary, %description expandable at SRPM build time.
[x]: MUST Package contains desktop file if it is a GUI application.
[x]: MUST Package installs a %{name}.desktop using desktop-file-install if
     there is such a file.
[-]: MUST Development files must be in a -devel package
[x]: MUST Package requires other packages for directories it uses.
[ ]: MUST Package uses nothing in %doc for runtime.
[x]: MUST Package is not known to require ExcludeArch.
[x]: MUST Permissions on files are set properly.
[x]: MUST Package does not contain duplicates in %files.
[ ]: MUST Package complies to the Packaging Guidelines
[x]: MUST Spec file lacks Packager, Vendor, PreReq tags.
[!]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf is only needed if supporting EPEL5
[-]: MUST Large documentation files are in a -doc subpackage, if required.
[x]: MUST If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %doc.
[x]: MUST License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. No licenses
     found. Please check the source files for licenses manually.
[x]: MUST Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: MUST Package is named using only allowed ascii characters.
[x]: MUST Package is named according to the Package Naming Guidelines.
[x]: MUST Package does not generate any conflict.
     Note: Package contains no Conflicts: tag(s)
[x]: MUST Package obeys FHS, except libexecdir and /usr/target.
[-]: MUST If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: MUST Package must own all directories that it creates.
[x]: MUST Package does not own files or directories owned by other packages.
[x]: MUST Package installs properly.
[x]: MUST Package is not relocatable.
[x]: MUST Requires correct, justified where necessary.
[x]: MUST Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: MUST Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: MUST Spec file is legible and written in American English.
[x]: MUST Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[-]: MUST Package contains systemd file(s) if in need.
[x]: MUST File names are valid UTF-8.
[x]: MUST Useful -debuginfo package or justification otherwise.
[x]: SHOULD Reviewer should test that the package builds in mock.
[!]: SHOULD Buildroot is not present
     Note: Invalid buildroot found:
     %{_tmppath}/%{valkyrie}-%{release}-root-%(%{__id_u} -n)
[!]: SHOULD Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: Clean is needed only if supporting EPEL5
[-]: SHOULD If the source package does not include license text(s) as a
     separate file from upstream, the packager SHOULD query upstream to
     include it.
[x]: SHOULD Dist tag is present.
[x]: SHOULD No file requires outside of /etc, /bin, /sbin, /usr/bin,
     /usr/sbin.
[-]: SHOULD Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[x]: SHOULD Package functions as described.
[x]: SHOULD Latest version is packaged.
[x]: SHOULD Package does not include license text files separate from
     upstream.
[-]: SHOULD Patches link to upstream bugs/comments/lists or are otherwise
     justified.
[x]: SHOULD SourceX tarball generation or download is documented.
[x]: SHOULD SourceX / PatchY prefixed with %{name}.
[x]: SHOULD SourceX is a working URL.
[-]: SHOULD Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: SHOULD Package should compile and build into binary rpms on all supported
     architectures.
[-]: SHOULD %check is present and all tests pass.
[?]: SHOULD Packages should try to preserve timestamps of original installed
     files.
[x]: SHOULD Spec use %global instead of %define.

Issues:
[!]: MUST Each %files section contains %defattr if rpm < 4.4
     Note: defattr(....) present in %files section. This is OK if packaging
     for EPEL5. Otherwise not needed
See: http://fedoraproject.org/wiki/Packaging/Guidelines#FilePermissions
[!]: MUST Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf is only needed if supporting EPEL5
See: None
[!]: MUST If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %doc.
See: http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text
[!]: MUST License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. No licenses
     found. Please check the source files for licenses manually.
See: http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#ValidLicenseShortNames

Rpmlint
-------
Checking: valkyrie-2.0.0-5.fc17.src.rpm
          valkyrie-2.0.0-5.fc17.x86_64.rpm
          valkyrie-debuginfo-2.0.0-5.fc17.x86_64.rpm
valkyrie.x86_64: E: incorrect-fsf-address /usr/share/doc/valkyrie-2.0.0/COPYING
valkyrie.x86_64: W: no-manual-page-for-binary valkyrie
valkyrie.x86_64: W: install-file-in-docs /usr/share/doc/valkyrie-2.0.0/INSTALL
3 packages and 0 specfiles checked; 1 errors, 2 warnings.


Rpmlint (installed packages)
----------------------------
# rpmlint valkyrie-debuginfo
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
# echo 'rpmlint-done:'

Requires
--------
valkyrie-2.0.0-5.fc17.x86_64.rpm (rpmlib, GLIBC filtered):
    
    libQtCore.so.4()(64bit)  
    libQtGui.so.4()(64bit)  
    libQtXml.so.4()(64bit)  
    libc.so.6()(64bit)  
    libgcc_s.so.1()(64bit)  
    libgcc_s.so.1(GCC_3.0)(64bit)  
    libm.so.6()(64bit)  
    libpthread.so.0()(64bit)  
    libstdc++.so.6()(64bit)  
    libstdc++.so.6(CXXABI_1.3)(64bit)  
    rtld(GNU_HASH)  
    valgrind >= 3.6.0

valkyrie-debuginfo-2.0.0-5.fc17.x86_64.rpm (rpmlib, GLIBC filtered):
    

Provides
--------
valkyrie-2.0.0-5.fc17.x86_64.rpm:
    
    valkyrie = 2.0.0-5.fc17
    valkyrie(x86-64) = 2.0.0-5.fc17

valkyrie-debuginfo-2.0.0-5.fc17.x86_64.rpm:
    
    valkyrie-debuginfo = 2.0.0-5.fc17
    valkyrie-debuginfo(x86-64) = 2.0.0-5.fc17

MD5-sum check
-------------
http://www.valgrind.org/downloads/valkyrie-2.0.0.tar.bz2 :
  CHECKSUM(SHA256) this package     : a70b9ffb2409c96c263823212b4be6819154eb858825c9a19aad0ae398d59b43
  CHECKSUM(SHA256) upstream package : a70b9ffb2409c96c263823212b4be6819154eb858825c9a19aad0ae398d59b43


Generated by fedora-review 0.2.2 (9f8c0e5) last change: 2012-08-09
Command line :/usr/bin/fedora-review -r -n valkyrie
External plugins:

Oh and I forgot to mention, that the issues listed by fedora-review are false positives, because you are intending to package for epel5 and the license thing is checked.

(In reply to comment #17)
> I made a complete review now, based on valkyrie-2.0.0-5.el6_3.src.rpm.
> 
> There are two remaining issues, but I am not 100% sure about them.
> 
> First: Package uses nothing in %doc for runtime. You moved the html
> documentation in %doc. It is shown at runtime when you click on Help ->
> Contents. I think this is not really using it at runtime. If these html
> files are missing, the Online Documentation will be empty.
>
If it works as expected, don't bother with it. Even it it fails to start the application, users will find the right doc folder according to the package name, and can open the html stuff in the browser of their choice.
Anyway, anything in %doc must not affect the runtime of an application, see the review guidelines.

> Second: %build honors applicable compiler flags or justifies otherwise. I do
> not see something like CFLAGS=... in your spec file and I am not familiar
> with qmake Makefile generation, environment CFLAGS set during build. If the
> package honors the compiler flags, tell me how it works :-).
> 
This is due to the macro %{?_smp_mflags}. Only in special cases, when CFLAGS, CXXFLAGS, OPTS or anything similar is harcoded in a pre-built Makefile, the appropriate definition has to be applied. Mostly the debug packages are empty if compiler flags are not applied correctly, but rpmlint takes care about this.

> I also do not know how to check: Packages should try to preserve timestamps
> of original installed files. It is only and I marked it with Not evaluated.
> As above, I would be thankful if someone could enlighten me howto check this.
> 
With automake, qmake and similar tools timestamps will be preserved anyway. When installing files manually, make it sure by using the "preserve" switches (install -p or cp -p).

@Sebastian, because you are sponsored in the packagers group now, you might assign this review request to you and complete the review.

Thanks Mario, for the explanation. All issues are resolved.

----------------

PACKAGE APPROVED

----------------

Oops, forgot to edit the assignee field :-).

Awesome, thanks for all the help guys!

New Package SCM Request
=======================
Package Name: valkyrie
Short Description: Graphical User Interface for Valgrind Suite
Owners: nathans
Branches: f16 f17 f18 el5 el6
InitialCC: mjw

Git done (by process-git-requests).

Packages for all requested branches have been built, but I cannot find anything in Bodhi. What's the matter?

Ah, thanks for the reminder Mario.  I'm not sure whether I've missed a step - usually at this point I would use the web interface to mark the packages as "submitted for testing" ... but, https://admin.fedoraproject.org/updates/search/valkyrie gives no results (for my other packages, this shows all the builds).

Have a missed setting something up?

thanks!

Go to https://admin.fedoraproject.org/updates/, log in and click on "New update" in the left pane. Then you can add the Koji builds and submit them for testing. Don't forget to submit the bug number and activate "Close this bug when update is stable". This makes sure that modifying and closing the review request will handled properly.

valkyrie-2.0.0-5.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/valkyrie-2.0.0-5.el6

valkyrie-2.0.0-5.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/valkyrie-2.0.0-5.fc18

valkyrie-2.0.0-5.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/valkyrie-2.0.0-5.fc17

valkyrie-2.0.0-5.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/valkyrie-2.0.0-5.fc16

Package valkyrie-2.0.0-5.fc18:
* should fix your issue,
* was pushed to the Fedora 18 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing valkyrie-2.0.0-5.fc18'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2012-17275/valkyrie-2.0.0-5.fc18
then log in and leave karma (feedback).

valkyrie-2.0.0-5.el6 has been pushed to the Fedora EPEL 6 stable repository.