Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/diorite.spec SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/diorite-0.3.3-0.1git7e26836.fc25.src.rpm Description: Diorite Library is a utility and widget library for Nuvola Player project based on GLib, GIO and GTK. Fedora Account System Username: martinkg The package builds only with the bundled waf version. The package was retired, old bugzilla review request https://bugzilla.redhat.com/show_bug.cgi?id=1158872
%changelog * Tue Apr 11 2017 Martin Gansser <martinkg> - 0.3.3-0.1git7e26836 - Update to 0.3.3-0.1git7e26836 rpmlint diorite.spec /home/martin/rpmbuild/SRPMS/diorite-0.3.3-0.1git7e26836.fc25.src.rpm /home/martin/rpmbuild/RPMS/x86_64/diorite-0.3.3-0.1git7e26836.fc25.x86_64.rpm /home/martin/rpmbuild/RPMS/x86_64/diorite-devel-0.3.3-0.1git7e26836.fc25.x86_64.rpm /home/martin/rpmbuild/RPMS/x86_64/diorite-debuginfo-0.3.3-0.1git7e26836.fc25.x86_64.rpm (none): E: error while reading /home/martin/rpmbuild/SRPMS/diorite-0.3.3-0.1git7e26836.fc25.src.rpm: 'str' object has no attribute 'decode' diorite.x86_64: W: no-soname /usr/lib64/libdioriteglib-0.3.so diorite.x86_64: W: no-soname /usr/lib64/libdioritegtk-0.3.so diorite.x86_64: W: no-manual-page-for-binary diorite-testgen-0.3 diorite-devel.x86_64: W: only-non-binary-in-usr-lib diorite-devel.x86_64: W: no-documentation 3 packages and 1 specfiles checked; 0 errors, 5 warnings. koji build --scratch rawhide ../SRPMS/diorite-0.3.3-0.1git7e26836.fc25.src.rpm Created task: 18954041 Task info: https://koji.fedoraproject.org/koji/taskinfo?taskID=18954041
Taking this for a review.
Several notes: * Is there any reason to package the git snapshot? Wouldn't it be easier to go with the latest official release (0.3.2). I am using some older version (0.3.1) of your diorite package and I don't observe any issues. Also, since there is not provided any API/ABI stability guarantee, I'd say that it would be better to package just stable versions and the projects using this library should always explicitly specify the diorite version. * I don't think the "Release" field is formatted according to the guidelines [1]. I should be 0.1.20171104git%{shortcommit0} IMO. * What is the reason to use the bundled version of "waf"? We have waf package in Fedora, is it possible to use it instead? * Wouldn't be %setup sufficient in place of %autosetup? * As far as I understand, the "waf configure" is using pkgconfig to check the proper configuration. Wouldn't it be more appropriate to use virtual provides such as "BuildRequires: pkgconfig(gtk+-3.0)" * I don't think you need vala-devel, since you are not going to extend Vala. The only thing you need is actually Vala compiler which is provided by "vala" package, but it might be better to "BuildRequires: %{_bindir}/valac" instead. * Since the application is later build using GCC, you should consider to add "BuildRequires: gcc" [2]. * There appears to be tests suite. Is there a chance to execute it during build? * The license should be just "BSD" shouldn't it? I can't see any code licensed under LGPLv3+ and LGPLv2+. * There is no license file included. Could you please query upstream to add one [3]? [1] https://fedoraproject.org/wiki/Packaging:Versioning#Snapshots [2] https://fedoraproject.org/wiki/Packaging:C_and_C++#BuildRequires_and_Requires [3] https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text
(In reply to Vít Ondruch from comment #3) > Several notes: > > * Is there any reason to package the git snapshot? Wouldn't it be easier to > go with the latest official release (0.3.2). I am using some older version > (0.3.1) of your diorite package and I don't observe any issues. Also, since > there is not provided any API/ABI stability guarantee, I'd say that it would > be better to package just stable versions and the projects using this > library should always explicitly specify the diorite version. I will take the stable diorite-0.3.1 now > > * I don't think the "Release" field is formatted according to the guidelines > [1]. I should be 0.1.20171104git%{shortcommit0} IMO. > > * What is the reason to use the bundled version of "waf"? We have waf > package in Fedora, is it possible to use it instead? I know this, but diorite need version 1.7.14 of waf, so i will use the bundle version. Fedora have version 1.9.7. > > * Wouldn't be %setup sufficient in place of %autosetup? i will take %setup because no patches are needed. > > * As far as I understand, the "waf configure" is using pkgconfig to check > the proper configuration. Wouldn't it be more appropriate to use virtual > provides such as "BuildRequires: pkgconfig(gtk+-3.0)" i will change it to: BuildRequires: gcc BuildRequires: pkgconfig(python3) BuildRequires: pkgconfig(gtk+-3.0) BuildRequires: %{_bindir}/valac > > * I don't think you need vala-devel, since you are not going to extend Vala. > The only thing you need is actually Vala compiler which is provided by > "vala" package, but it might be better to "BuildRequires: %{_bindir}/valac" > instead. done > > * Since the application is later build using GCC, you should consider to add > "BuildRequires: gcc" [2]. done > > * There appears to be tests suite. Is there a chance to execute it during > build? don't know what is the correct command ? > > * The license should be just "BSD" shouldn't it? I can't see any code > licensed under LGPLv3+ and LGPLv2+. will change it to BSD only. > > * There is no license file included. Could you please query upstream to add > one [3]? opened upstream ticket. https://github.com/tiliado/diorite/issues/10
Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/diorite.spec SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/diorite-0.3.1-1.fc25.src.rpm %changelog * Sat Jan 21 2017 Martin Gansser <martinkg> - 0.3.1-1 - Update to stable 0.3.1 - Correct license tag to BSD - Use virtual provides for BR - Add LICENSE file from master branch
Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/diorite.spec SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/diorite-0.3.3-1.fc25.src.rpm %changelog * Sun Apr 16 2017 Martin Gansser <martinkg> - 0.3.3-1 - Update to stable 0.3.3
Thanks for updated package. * The stable release does not build on Rawhide :/ It seems the medicine is available here [1]. * The installation should be run with "--no-ldconfig" option to prevent "/sbin/ldconfig: Can't create temporary cache file /etc/ld.so.cache~: Permission denied" error. >> * What is the reason to use the bundled version of "waf"? We have waf >> package in Fedora, is it possible to use it instead? > > I know this, but diorite need version 1.7.14 of waf, so i will use the bundle > version. Fedora have version 1.9.7 Well, there appears to be one minor silly issue. I sent PR fixing this upstream [2]. You can either apply my patch or go with "VALAFLAGS=--quiet" as simplest workaround. Here are the changes I tried: ~~~ $ diff -u diorite.spec{.orig,} --- diorite.spec.orig 2017-04-18 14:46:44.487155913 +0200 +++ diorite.spec 2017-04-18 14:48:35.329070349 +0200 @@ -15,6 +15,7 @@ BuildRequires: pkgconfig(python3) BuildRequires: pkgconfig(gtk+-3.0) BuildRequires: %{_bindir}/valac +BuildRequires: %{_bindir}/waf %description @@ -31,16 +32,19 @@ %prep %setup -qn %{name}-%{version} +rm ./waf +rm -rf ./.waf + cp -p %{SOURCE1} LICENSE %build -./waf configure --prefix=%{_prefix} \ +VALAFLAGS=--quiet waf configure --prefix=%{_prefix} \ --libdir=%{_libdir} -./waf build --prefix=%{_prefix} \ +waf build --prefix=%{_prefix} \ --libdir=%{_libdir} %install -./waf install --prefix=%{_prefix} \ +waf install --prefix=%{_prefix} \ --libdir=%{_libdir} \ --destdir=%{buildroot} ~~~ Unfortunately, the "waf install" triggers rebuild of some files and I was not lucky yet to find out what is the reason :/ >> * There is no license file included. Could you please query upstream to add >> one [3]? > > opened upstream ticket. > https://github.com/tiliado/diorite/issues/10 Thx. Unfortunately, you included wrong LICENSE file. The file you have included is GPL instead of BSD. Since the guidelines requires just to ask upstream but not include the file unless the file is provided in tarball, I'd suggest just to drop the file and wait until the LICENSE file is included in the upstream tarball. >> * There appears to be tests suite. Is there a chance to execute it during >> build? > > don't know what is the correct command ? It seems that "--with-experimental-api" configuration flag enables build of test suite. Later it can be executed by "LD_LIBRARY_PATH=./build ./build/run-dioritetests-0.3". But the experimental features requires "sqlite-devel" and I am not sure what is the impact on the installed libraries ... [1] https://github.com/tiliado/diorite/commit/7e2683628d04e6f4f4a7cf4bc48a0af3937fbb1b [2] https://github.com/tiliado/diorite/pull/11
(In reply to Vít Ondruch from comment #7) > Thanks for updated package. > > * The stable release does not build on Rawhide :/ It seems the medicine is > available here [1]. > [1] > https://github.com/tiliado/diorite/commit/ > 7e2683628d04e6f4f4a7cf4bc48a0af3937fbb1b should i generate a patch to allow build on Rawhide ? > >> * There appears to be tests suite. Is there a chance to execute it during > >> build? > > > > don't know what is the correct command ? > > It seems that "--with-experimental-api" configuration flag enables build of > test suite. Later it can be executed by "LD_LIBRARY_PATH=./build > ./build/run-dioritetests-0.3". But the experimental features requires > "sqlite-devel" and I am not sure what is the impact on the installed > libraries ... > i changed this on the spec file to build the test-suite: ... %build VALAFLAGS=--quiet waf configure --with-experimental-api \ --prefix=%{_prefix} \ --libdir=%{_libdir} waf build --prefix=%{_prefix} \ --libdir=%{_libdir} %install waf install --no-ldconfig --prefix=%{_prefix} \ --libdir=%{_libdir} \ --destdir=%{buildroot} %check LD_LIBRARY_PATH=./build ./build/run-dioritetests-0.3 .... but the compilaton fails with: [ 62/114] Compiling build/main.c [ 63/114] Compiling build/KeyValueStorage.c [ 64/114] Compiling build/Dialogs.c [ 65/114] Compiling build/run-dioritetests-0.3.c In file included from ./dioritetests-0.3.h:8:0, from run-dioritetests-0.3.c:9: ./dioriteglib-0.3.h:14:38: fatal error: gio/gfiledescriptorbased.h: No such file or directory #include <gio/gfiledescriptorbased.h> ^ compilation terminated. glib-devel sqlite-devel and python3-pyparsingis is installed rpm -qf /usr/include/gio-unix-2.0/gio/gfiledescriptorbased.h glib2-devel-2.50.3-1.fc25.x86_64
(In reply to mgansser from comment #8) > (In reply to Vít Ondruch from comment #7) > > Thanks for updated package. > > > > * The stable release does not build on Rawhide :/ It seems the medicine is > > available here [1]. > > > [1] > > https://github.com/tiliado/diorite/commit/ > > 7e2683628d04e6f4f4a7cf4bc48a0af3937fbb1b > > should i generate a patch to allow build on Rawhide ? Yes, Rawhide is the first build target ... > > >> * There appears to be tests suite. Is there a chance to execute it during > > >> build? > > > > > > don't know what is the correct command ? > > > > It seems that "--with-experimental-api" configuration flag enables build of > > test suite. Later it can be executed by "LD_LIBRARY_PATH=./build > > ./build/run-dioritetests-0.3". But the experimental features requires > > "sqlite-devel" and I am not sure what is the impact on the installed > > libraries ... > > > > i changed this on the spec file to build the test-suite: > ... > %build > VALAFLAGS=--quiet waf configure --with-experimental-api \ > --prefix=%{_prefix} \ > --libdir=%{_libdir} > waf build --prefix=%{_prefix} \ > --libdir=%{_libdir} > > %install > waf install --no-ldconfig --prefix=%{_prefix} \ > --libdir=%{_libdir} \ > --destdir=%{buildroot} > > %check > LD_LIBRARY_PATH=./build ./build/run-dioritetests-0.3 > .... > > but the compilaton fails with: > > [ 62/114] Compiling build/main.c > [ 63/114] Compiling build/KeyValueStorage.c > [ 64/114] Compiling build/Dialogs.c > [ 65/114] Compiling build/run-dioritetests-0.3.c > In file included from ./dioritetests-0.3.h:8:0, > from run-dioritetests-0.3.c:9: > ./dioriteglib-0.3.h:14:38: fatal error: gio/gfiledescriptorbased.h: No such > file or directory > #include <gio/gfiledescriptorbased.h> > ^ > compilation terminated. > > glib-devel sqlite-devel and python3-pyparsingis is installed > > rpm -qf /usr/include/gio-unix-2.0/gio/gfiledescriptorbased.h > glib2-devel-2.50.3-1.fc25.x86_64 It seems that the author runs the tests either under windows or on his environment, so there is missing this bit: ~~~ --- wscript.orig 2017-04-18 17:10:41.940079460 +0200 +++ wscript 2017-04-18 15:46:27.868186780 +0200 @@ -304,8 +305,8 @@ ctx.program( target = RUN_DIORITE_TESTS, source = [ctx.path.find_or_declare("%s.vala" % RUN_DIORITE_TESTS)], - packages = "gio-2.0 glib-2.0", - uselib = "GIO GLIB", + packages = "gio-2.0 glib-2.0 gio-unix-2.0", + uselib = "GIO GLIB UNIXGIO", use = [DIORITE_GLIB, DIORITE_GTK, DIORITE_DB, DIORITE_TESTS], vala_defines = vala_defines, defines = ['G_LOG_DOMAIN="DioriteTests"'], ~~~ But this hunk is not upstreamable. There would need to be some Windows specific check, similar to [1]. [1] https://github.com/tiliado/diorite/blob/master/wscript#L227
Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/diorite.spec SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/diorite-0.3.3-2.fc25.src.rpm %changelog * Thu Apr 20 2017 Martin Gansser <martinkg> - 0.3.3-2 - Add BR %%{_bindir}/waf - Add BR sqlite-devel - Add BR python3-pyparsing - Add --no-ldconfig flag to prevent - Dropped LICENSE file because it's wrong - Add wscript.patch - Add compile-on-rawhide.patch koji build --scratch rawhide ../SRPMS/diorite-0.3.3-2.fc25.src.rpm https://koji.fedoraproject.org/koji/taskinfo?taskID=19093414
(In reply to mgansser from comment #10) > - Add BR python3-pyparsing Testing on Rawhide, this is not needed. * There is one minor nit reported by rpmlint: diorite.x86_64: E: wrong-script-interpreter /usr/bin/diorite-testgen-0.3 /usr/bin/env python3 BTW I reported the issue with "waf install" to upstream: https://github.com/waf-project/waf/issues/1948 Otherwise the package looks good => APPROVED.
(In reply to Vít Ondruch from comment #11) > (In reply to mgansser from comment #10) > > - Add BR python3-pyparsing > > Testing on Rawhide, this is not needed. should i use a if clause or what you mean ? %if 0%{?fedora} <= 26 BuildRequires: python3-pyparsing %endif ... %if 0%{?fedora} <= 26 %check LD_LIBRARY_PATH=./build ./build/run-dioritetests-%{major_version} %endif > > * There is one minor nit reported by rpmlint: > > diorite.x86_64: E: wrong-script-interpreter /usr/bin/diorite-testgen-0.3 > /usr/bin/env python3 corrected > > BTW I reported the issue with "waf install" to upstream: > > https://github.com/waf-project/waf/issues/1948 yes thanks, i saw the ticket already > > Otherwise the package looks good => APPROVED. @Vit many thanks for the review Spec URL: https://martinkg.fedorapeople.org/Review/SPECS/diorite.spec SRPM URL: https://martinkg.fedorapeople.org/Review/SRPMS/diorite-0.3.3-3.fc25.src.rpm %changelog * Thu Apr 20 2017 Martin Gansser <martinkg> - 0.3.3-3 - fixed wrong-script-end-of-line-encoding
(In reply to mgansser from comment #12) > (In reply to Vít Ondruch from comment #11) > > (In reply to mgansser from comment #10) > > > - Add BR python3-pyparsing > > > > Testing on Rawhide, this is not needed. > > should i use a if clause or what you mean ? If you are going to push Nuvola/Diorite into older Fedoras, then this is probably good idea. At least you will know in the future that the dependency can be dropped > %if 0%{?fedora} <= 26 > BuildRequires: python3-pyparsing > %endif > > ... You don't need the snipped bellow, do you? The test suite should always pass, it just needs the dependency on older Fedoras. > %if 0%{?fedora} <= 26 > %check > LD_LIBRARY_PATH=./build ./build/run-dioritetests-%{major_version} > %endif
(In reply to Vít Ondruch from comment #13) > (In reply to mgansser from comment #12) > > (In reply to Vít Ondruch from comment #11) > > > (In reply to mgansser from comment #10) > > > > - Add BR python3-pyparsing > > > > > > Testing on Rawhide, this is not needed. > > > > should i use a if clause or what you mean ? > > If you are going to push Nuvola/Diorite into older Fedoras, then this is > probably good idea. At least you will know in the future that the dependency > can be dropped > > > %if 0%{?fedora} <= 26 > > BuildRequires: python3-pyparsing > > %endif > > > > ... > > You don't need the snipped bellow, do you? The test suite should always > pass, it just needs the dependency on older Fedoras. > > > %if 0%{?fedora} <= 26 > > %check > > LD_LIBRARY_PATH=./build ./build/run-dioritetests-%{major_version} > > %endif done if i request diorite as new package on https://admin.fedoraproject.org/pkgdb/request/package/ i get the message: There is already a package named: rpms/diorite
(In reply to mgansser from comment #14) > if i request diorite as new package on > https://admin.fedoraproject.org/pkgdb/request/package/ > > i get the message: > > There is already a package named: rpms/diorite This is the relevant guide: https://fedoraproject.org/wiki/Orphaned_package_that_need_new_maintainers#Claiming_Ownership_of_a_Retired_Package You should follow the steps 4 and 5 now.
(In reply to Vít Ondruch from comment #15) > (In reply to mgansser from comment #14) > > if i request diorite as new package on > > https://admin.fedoraproject.org/pkgdb/request/package/ > > > > i get the message: > > > > There is already a package named: rpms/diorite > > This is the relevant guide: > > https://fedoraproject.org/wiki/ > Orphaned_package_that_need_new_maintainers#Claiming_Ownership_of_a_Retired_Pa > ckage > > You should follow the steps 4 and 5 now. unretire request: https://pagure.io/releng/issue/6758
can' push the new package due access rights. [martin@fc25 diorite]$ fedpkg new-sources diorite-0.3.3.tar.gz File already uploaded: diorite-0.3.3.tar.gz Source upload succeeded. Don't forget to commit the sources file [martin@fc25 diorite]$ git add wscript.patch compile-on-rawhide.patch [martin@fc25 diorite]$ fedpkg diff [martin@fc25 diorite]$ fedpkg commit -p -c [master 1906ad7] fixed wrong-script-end-of-line-encoding 4 files changed, 286 insertions(+) create mode 100644 .gitignore create mode 100644 compile-on-rawhide.patch create mode 100644 sources create mode 100644 wscript.patch FATAL: W any rpms/diorite martinkg DENIED by fallthru (or you mis-spelled the reponame) fatal: Could not read from remote repository. Please make sure you have the correct access rights and the repository exists. Could not execute commit: Command '['git', 'push']' returned non-zero exit status 128 how can i solve this ?
package has been built successfully on f25, f26 and rawhide.