On Thu, Sep 05, 2013 at 09:21:31AM +0200, Miroslav Suchy wrote: > Hi, > I would like to ask on your opinion on > https://bugzilla.redhat.com/show_bug.cgi?id=912960#c25 > > Mamoru have in spec in %check section: > ruby -Ilib:test ./test/run-test.rb || echo "Please investigate this" > > I'm saying that it should not be waived this way as it waive all failure. > I'm saying that if there is no failure, it should be just: > ruby -Ilib:test ./test/run-test.rb > and if there happened some problem which need upstream attention and > could not be fixed immediatelly, it should be replaced with something > like: > FILE=$(mktemp) > ruby -Ilib:test ./test/run-test.rb | tee $FILE || : > # test foo is failing, reported as http://foo/issue/1 > cat $FILE | grep "4 tests, 4 assertions, 1 failures, 0 errors, 0 > pendings, 0 omissions, 0 notifications > > Mamoru disagree. For me it is blocker. > I would like to ask for your opinions. Which way is correct and > should it be blocker for review? > As mschwendt and scop have replied, at the moment there is nothing in the guidelines to prevent disabling of the entire testsuite but reviewers are able to exercise their best judgement here. When I run across failures in testsuites in my own packages my order of fixing is: 1) Try to fix the code and submit the patch upstream 1b) If I can't figure out the issue, submit the problem upstream and hope they'll come up with a solution quickly. 2) If upstream's "quickly" isn't quick enough, I may disable just that one test. Sometimes this can be done with a command line switch to exclude a named test. Other times I have to resort to a temporary patch to remove this. Comment why the test is disabled. This depends on me feeling that the test isn't instrumental to the package's functionality (for me, this falls under: "SHOULD: The reviewer should test that the package functions as described. A package should not segfault instead of running, for example." from https://fedoraproject.org/wiki/Packaging:ReviewGuidelines ) For my own packages, I have rejected build fixes that disable the entire testsuite for a few failing tests. When reviewing packages, I do require people to follow a variation of what I mention above. I provide patches to disable specific tests if that is necessary. I've never encountered a situation where the packager actively wants to disable the whole suite, however, so I don't know whether I would continue the review in that situation or not. For https://bugzilla.redhat.com/show_bug.cgi?id=912960#c25 I do agree with Mamoru that grep is not a good solution. I would propose a patch in this case. This particular test failure looks like it's a difference in rounding the results which the GDK docs appear to allow: https://developer.gnome.org/gdk3/stable/gdk3-RGBA-Colors.html#gdk-rgba-to-string So I'm thinking this is a test case that's too-strict. I'm neither a ruby nor a gtk guy but I think I have a patch that you could submit upstream based on this. I'll attach it here. Also, not disabling the tests en masse seems to be a good idea -- I just tried to scratch build this on f20 and found that there's been changes that cause a different error to be thrown. I'll attach a patch for that as well (note -- you may have to conditionalize which Fedora releases that gets applied on. I'm not certain when the API changed.) And just for good measure, I'll attach the spec file I used as well Successful build: http://koji.fedoraproject.org/koji/taskinfo?taskID=5900043 -Toshio
diff -up gdk3/gdk3-1.2.6/lib/gdk3/base.rb.new-gnome gdk3/gdk3-1.2.6/lib/gdk3/base.rb
--- gdk3/gdk3-1.2.6/lib/gdk3/base.rb.new-gnome 2013-09-05 08:11:27.703032716 -0700
+++ gdk3/gdk3-1.2.6/lib/gdk3/base.rb 2013-09-05 08:11:42.487891416 -0700
@@ -10,7 +10,7 @@ require 'gdk_pixbuf2'
base_dir = Pathname.new(__FILE__).dirname.dirname.dirname.expand_path
vendor_dir = base_dir + "vendor" + "local"
vendor_bin_dir = vendor_dir + "bin"
-GLib.prepend_environment_path(vendor_bin_dir)
+GLib.prepend_dll_path(vendor_bin_dir)
begin
major, minor, _ = RUBY_VERSION.split(/\./)
require "#{major}.#{minor}/gdk3.so"
diff -up gdk3/gdk3-1.2.6/test/test-gdk-rgba.rb.precision gdk3/gdk3-1.2.6/test/test-gdk-rgba.rb
--- gdk3/gdk3-1.2.6/test/test-gdk-rgba.rb.precision 2013-09-05 08:38:41.735194176 -0700
+++ gdk3/gdk3-1.2.6/test/test-gdk-rgba.rb 2013-09-05 08:46:10.125305290 -0700
@@ -19,6 +19,6 @@
class TestGdkRGBA < Test::Unit::TestCase
def test_to_s
rgba = Gdk::RGBA.new(0.1, 0.2, 0.3, 0.5)
- assert_equal("rgba(26,51,77,0.5)", rgba.to_s)
+ assert_match(/rgba\((26|25),51,(77|76),0.5\)/, rgba.to_s)
end
end
%global header_dir %{ruby_vendorarchdir}
%global gem_name gdk3
%global glib_min_ver 1.2.1
%if 0%{?fedora} < 19
%global rubyabi 1.9.1
%endif
Summary: Ruby binding of GDK-3.x
Name: rubygem-%{gem_name}
Version: 1.2.6
Release: 1%{?dist}
Group: Development/Languages
# Various files in gem
License: LGPLv2+
URL: http://ruby-gnome2.sourceforge.jp/
Source0: http://rubygems.org/gems/%{gem_name}-%{version}.gem
# https://raw.github.com/ruby-gnome2/ruby-gnome2/master/gdk3/COPYING.LIB
# Renamed to avoid overwrite on SOURCE dir
Source1: COPYING.LIB.gdk3
Patch0: rubygem-gdk3-new-glib.patch
Patch1: rubygem-gdk3-precision.patch
%if 0%{?fedora} >= 19
Requires: ruby(release)
BuildRequires: ruby(release)
%else
Requires: ruby(abi) = %{rubyabi}
Requires: ruby
BuildRequires: ruby(abi) = %{rubyabi}
BuildRequires: ruby
%endif
Requires: ruby(rubygems)
Requires: rubygem(glib2) >= %{glib_min_ver}
Requires: rubygem(atk)
Requires: rubygem(pango)
Requires: rubygem(gdk_pixbuf2)
BuildRequires: ruby-devel
BuildRequires: rubygems-devel
BuildRequires: rubygem-glib2-devel >= %{glib_min_ver}
BuildRequires: rubygem-pango-devel
BuildRequires: gtk3-devel
# %%check
BuildRequires: rubygem(gdk_pixbuf2)
BuildRequires: rubygem(test-unit)
BuildRequires: rubygem(test-unit-notify)
Provides: rubygem(%{gem_name}) = %{version}-%{release}
%description
Ruby/GDK3 is a Ruby binding of GDK-3.x.
%package devel
Summary: Ruby/GLib development environment
Group: Development/Languages
Requires: %{name}%{?isa} = %{version}-%{release}
Requires: gtk3-devel%{?isa}
Requires: ruby-devel%{?isa}
%description devel
Header files and libraries for building a extension library for the
rubygem-%{gem_name}
%package doc
Summary: Documentation for %{name}
Group: Documentation
Requires: %{name} = %{version}-%{release}
BuildArch: noarch
%description doc
Documentation for %{name}
%prep
%setup -q -c -T
TOPDIR=$(pwd)
mkdir tmpunpackdir
pushd tmpunpackdir
gem unpack %{SOURCE0}
cd %{gem_name}-%{version}
# Needed on at least f20
%patch0 -p2 -b .new-glib
# Different precision/rounding on different systems
%patch1 -p2 -b .precision
# Permission
find . -name \*.rb -print0 | xargs --null chmod 0644
gem specification -l --ruby %{SOURCE0} > %{gem_name}.gemspec
# Add license text
install -cpm 644 %{SOURCE1} ./COPYING.LIB
sed -i -e '/files =/s|\("Rakefile",\)|\1 "COPYING.LIB", |' \
%{gem_name}.gemspec
gem build %{gem_name}.gemspec
mv %{gem_name}-%{version}.gem $TOPDIR
popd
rm -rf tmpunpackdir
%build
mkdir -p .%{gem_dir}
export CONFIGURE_ARGS="--with-cflags='%{optflags} -Werror-implicit-function-declaration'"
export CONFIGURE_ARGS="$CONFIGURE_ARGS --with-pkg-config-dir=$(pwd)%{_libdir}/pkgconfig"
%gem_install
%install
mkdir -p %{buildroot}%{gem_dir}
cp -a .%{gem_dir}/* \
%{buildroot}%{gem_dir}/
# move header files, C extension files to the correct directory
pushd %{buildroot}
mkdir -p .%{header_dir}
mv .%{gem_instdir}/lib/*.h .%{header_dir}/
mkdir -p .%{gem_extdir_mri}/lib
mv .%{gem_instdir}/lib/%{gem_name}.so .%{gem_extdir_mri}/lib/
popd
# move pkgconfig file
mkdir %{buildroot}%{_libdir}/pkgconfig
install -cpm 644 ./%{_libdir}/pkgconfig/*.pc \
%{buildroot}%{_libdir}/pkgconfig/
# Cleanups
pushd %{buildroot}
rm -rf .%{gem_instdir}/ext/
rm -f .%{gem_instdir}/extconf.rb
popd
%check
pushd .%{gem_instdir}
# kill unneeded make process
rm -rf ./TMPBINDIR
mkdir ./TMPBINDIR
pushd ./TMPBINDIR
ln -sf /bin/true make
export PATH=$(pwd):$PATH
popd
%if 0%{?fedora} <= 18
sed -i.utest \
-e '\@^require@s|^.*test-unit.*$|gem "test-unit"\nrequire "test/unit"|' \
test/gdk-test-utils.rb
%endif
ruby -Ilib:test ./test/run-test.rb
popd
%files
%doc %{gem_instdir}/[A-Z]*
%exclude %{gem_instdir}/Rakefile
%dir %{gem_instdir}/
%dir %{gem_instdir}/lib/
%{gem_instdir}/lib/%{gem_name}.rb
%dir %{gem_instdir}/lib/%{gem_name}/
%{gem_instdir}/lib/%{gem_name}/*.rb
%dir %{gem_extdir_mri}/
%dir %{gem_extdir_mri}/lib/
%{gem_extdir_mri}/lib/%{gem_name}.so
%exclude %{gem_cache}
%{gem_spec}
%files devel
%{_libdir}/pkgconfig/ruby-%{gem_name}.pc
%{header_dir}/rbgdk3.h
%{header_dir}/rbgdk3conversions.h
%files doc
%doc %{gem_docdir}/
%exclude %{gem_instdir}/test/
%changelog
* Mon Apr 29 2013 Mamoru TASAKA <mtasaka@xxxxxxxxxxxxxxxxx> - 1.2.6-1
- 1.2.6
* Fri Mar 22 2013 Mamoru TASAKA <mtasaka@xxxxxxxxxxxxxxxxx> - 1.2.3-1
- 1.2.3
* Mon Feb 18 2013 Mamoru TASAKA <mtasaka@xxxxxxxxxxxxxxxxx> - 1.2.1-1
- Initial package
Attachment:
pgpSDZ41cgTwq.pgp
Description: PGP signature
-- packaging mailing list packaging@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/packaging
