Have you checked the PR?

Can you comment on the differences- to avoid duplicate PRs?

You also have several merge commits.

FTR - this is duplicate of PR#1.

Hi Pavel,

Thanks for the review.

You also have several merge commits.

Thanks, have squashed.

FTR - this is duplicate of PR#1.

This is based on my original spec file from the bugzilla report as Vit referenced in your PR. I've also cleaned things up a bit wrt ruby macros. I saw your commit but wasn't sure if you were actively working on it.

Cheers
Chris

So, the difference to my PR is(comments included):

diff --git a/rubygem-prawn.spec b/rubygem-prawn.spec
index 83bcaff..b75f4a8 100644
--- a/rubygem-prawn.spec
+++ b/rubygem-prawn.spec
@@ -4,17 +4,18 @@ Summary: A fast and nimble PDF generator for Ruby
 Name: rubygem-%{gem_name}
 Version: 2.2.2
 Release: 1%{?dist}
+Group: Development/Languages

Please remove Group, it's no longer used.

 # afm files are licensed by APAFML, the rest of package is GPLv2 or GPLv3 or Ruby
 License: (GPLv2 or GPLv3 or Ruby) and APAFML
 URL: http://prawnpdf.org
 Source0: http://rubygems.org/gems/%{gem_name}-%{version}%{?prever}.gem
 BuildRequires: ruby(release)
 BuildRequires: rubygems-devel >= 1.3.6
-BuildRequires: rubygem(rspec)
-BuildRequires: rubygem(ttfunk) >= 1.4
-BuildRequires: rubygem(pdf-reader) >= 1.2.0
-BuildRequires: rubygem(pdf-inspector) >= 1.2.0
-BuildRequires: rubygem(pdf-core) >= 0.6.0
+BuildRequires: rubygem(rspec) >= 3.0
+BuildRequires: rubygem(ttfunk) >= 1.5
+BuildRequires: rubygem(pdf-reader) >= 1.4.0
+BuildRequires: rubygem(pdf-inspector) >= 1.2.1
+BuildRequires: rubygem(pdf-core) >= 0.7.0

You're enforcing more restrictive BRs. There's really no reason to change those, as long as the build succeeds.
This is, however, a matter of opinion and does not affect anything I know of.

 BuildArch: noarch

 %description
@@ -44,6 +45,7 @@ Here are some of the important features we provide:

 %package doc
 Summary: Documentation for %{name}
+Group: Documentation

Please see the comment above.

 Requires: %{name} = %{version}-%{release}
 BuildArch: noarch

@@ -51,12 +53,12 @@ BuildArch: noarch
 Documentation for %{name}

 %prep
-gem unpack %{SOURCE0}
-%setup -q -D -T -n %{gem_name}-%{version}%{?prever}
-gem spec %{SOURCE0} -l --ruby > %{gem_name}.gemspec
+%setup -q -n %{gem_name}-%{version}
+%gemspec_remove_dep -g pdf-core "~> 0.7.0"
+%gemspec_add_dep -g pdf-core

 %build
-gem build %{gem_name}.gemspec
+gem build ../%{gem_name}-%{version}.gemspec

 %gem_install -n %{gem_name}-%{version}%{?prever}.gem

@@ -68,12 +70,13 @@ cp -a .%{gem_dir}/* \
 %check
 pushd .%{gem_instdir}
 sed -i "/^require 'bundler'/d" ./spec/spec_helper.rb
-sed -i '/^Bundler.setup/d' ./spec/spec_helper.rb
+sed -i "/^Bundler.setup/d" ./spec/spec_helper.rb

-# There are missing font files required by test suite.
+# There are missing font and image files required by test suite.
+# These are not bundled in the gem therefore some failures occur.
 rspec spec \
   | tee /dev/stderr \
-  | grep '837 examples, 108 failures'
+  | grep '850 examples, 193 failures'

Does the test suite pass for you, locally, with the font and image files included?

 popd

 %files
@@ -98,8 +101,8 @@ popd
 %doc %{gem_instdir}/manual

 %changelog
-* Tue Oct 30 2018 Pavel Valena <pvalena@redhat.com> - 2.2.2-1
-- Update to Prawn 2.2.2.
+* Tue Nov 06 2018 Christopher Brown <chris.brown@redhat.com> - 2.2.2-1
+- Update to 2.2.2

 * Sat Jul 14 2018 Fedora Release Engineering <releng@fedoraproject.org> - 2.1.0-6
 - Rebuilt for https://fedoraproject.org/wiki/Fedora_29_Mass_Rebuild

Otherwise it looks fine, thanks!
Will you look at the advised changes?

Does the test suite pass for you, locally, with the font and image files included?

I've checked myself, as follows.

There's missing BR: rubygem(prawn-manual_builder), which rspec complaints about, however adding that dependency does not result fix the relevant failing test:

  1) Prawn manual contains no unexpected changes
     Failure/Error: expect(hash).to eq MANUAL_HASH

       expected: "b55f154c9093c60f38051c75920c8157c775b946b0c77ffafc0a8a634ad5401e8ceafd0b96942839f82bacd
726a690af3fdd1fd9e185616f67c6c0edfcfd0460"
            got: "2bfb6ab4c79bdb6480d6fe480e3c00844be77f3651515a863a478e2aacbb69041126696b45b8effdef8cc0f
77b1f76b5c2b80c928fa6984a0f504b07b2850216"

       (compared using ==)
     # ./spec/manual_spec.rb:30:in `block (3 levels) in <top (required)>'

I assume that's just some arbitrary binary data check we need not investigate.

Regarding the missing files in 'data': images/, few fonts/* files, and shift_jis_text.txt. Copying those from their current master results in 104 failures.

So, the the majority of the issues persists and the breakages seem to me to be caused by incompatible pdf_reader and/or pdf_inspector versions with this test suite. In Fedora, both are in the lastest version available on rubygems.org.

All the errors are same AFAICT:

105) Prawn::Text#formatted_text draws text
       Failure/Error: text = PDF::Inspector::Text.analyze(pdf.render)

       NoMethodError:
         undefined method `to_utf8' for nil:NilClass
       # /usr/share/gems/gems/pdf-inspector-1.3.0/lib/pdf/inspector/text.rb:41:in `show_text'
       # /usr/share/gems/gems/pdf-inspector-1.3.0/lib/pdf/inspector/text.rb:47:in `show_text_with_positioning'
       # /usr/share/gems/gems/pdf-reader-2.1.0/lib/pdf/reader/page.rb:160:in `block in callback'
       # /usr/share/gems/gems/pdf-reader-2.1.0/lib/pdf/reader/page.rb:159:in `each'
       # /usr/share/gems/gems/pdf-reader-2.1.0/lib/pdf/reader/page.rb:159:in `callback'
       # /usr/share/gems/gems/pdf-reader-2.1.0/lib/pdf/reader/page.rb:146:in `content_stream'
       # /usr/share/gems/gems/pdf-reader-2.1.0/lib/pdf/reader/page.rb:111:in `walk'
       # /usr/share/gems/gems/pdf-inspector-1.3.0/lib/pdf/inspector.rb:19:in `block (2 levels) in analyze'
       # /usr/share/gems/gems/pdf-inspector-1.3.0/lib/pdf/inspector.rb:18:in `each'
       # /usr/share/gems/gems/pdf-inspector-1.3.0/lib/pdf/inspector.rb:18:in `block in analyze'
       # /usr/share/gems/gems/pdf-reader-2.1.0/lib/pdf/reader.rb:161:in `open'
       # /usr/share/gems/gems/pdf-inspector-1.3.0/lib/pdf/inspector.rb:17:in `analyze'
       # ./spec/prawn/text_with_inline_formatting_spec.rb:12:in `block (3 levels) in <top (required)>'

The error is caused by @state.current_font returning nil(here) which AFAICT means that the current font is invalid.

Could you investigate that?

Hi Pavel,

Could you investigate that?

Sure, I've had a look and I think we will need to move to a git checkout as Francois suspected we might.

I don't get these errors when running it from the latest master tarball, just the usual ones about missing fonts and images as before. I have an updated spec but kerberos seems to be down at the moment so will push tomorrow.

Thanks for reviewing.

If the error can be fixed by some small change, we could still stay on stable release and ship a patch instead. Also, if there are test suite fixes, we could apply them all as a patch to the test suite- that could help a lot.

(ugh, I appear to have edited your comment, apologies)

I was being lazy, I think. I've git bisected to:

https://github.com/prawnpdf/prawn/commit/c504ae4e683017d7afadece084734a9190230cd8

and the following appears to fix:

diff --git a/lib/prawn/font.rb b/lib/prawn/font.rb
index abc7cba1..a0070020 100644
--- a/lib/prawn/font.rb
+++ b/lib/prawn/font.rb
@@ -389,7 +389,7 @@ module Prawn
end

 def identifier_for(subset) #:nodoc:

• "#{@identifier}.#{subset}"

• "#{@identifier}.#{subset}".to_sym
  end

  def inspect #:nodoc:

Would appreciate if you could test also.

Have tested this, builds fine:

https://koji.fedoraproject.org/koji/taskinfo?taskID=30743351

Local tests are happy.

Also the GPL linting issue is fixed upstream:

https://github.com/prawnpdf/prawn/commit/ba6aba2f0a2d566bda5543953e63e81ed9461533

Any other comments please let me know otherwise will merge this soon.

Thanks!

It is always a good idea to comment on what is this patch good for and especially where is it coming from.

BTW the patch itself is not part of the PR.

@vondruch thanks, I have added comment to spec file. I've uploaded the patch with the new source, what am I missing?

I've uploaded the patch with the new source, what am I missing?

This is not the best approach, unfortunately. The patches are typically stored directly in git. You probably used "fedpkg new-sourcer", but it is much more convenient to use "fedpkg import", which does the right things for you.

And still, there is no URL to the original location of the patch. Look at this commit I did moments ago what I mean:

https://src.fedoraproject.org/rpms/ruby/c/c80ecd9db905f328079a9c8afee70a34e1dcc18c?branch=master

I've uploaded the patch with the new source, what am I missing?

This is not the best approach, unfortunately. The patches are typically stored directly in git. You probably used "fedpkg new-sourcer", but it is much more convenient to use "fedpkg import", which does the right things for you.

Ok, thanks, that's useful information.

And still, there is no URL to the original location of the patch.

Thanks, have added this.

LGTM. Let's see what @pvalena thinks about it.

It would be good to know what dependency exactly (here in the comment).

You're adding pdf-core BuildRequire restriction + BuildRequires: rubygem(pdf-core) >= 0.7.0.

IMO it would be good to keep the same one here(IOW >= 0.7.0).

Done.

LGTM, thanks!