Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ISLANDORA-2053: Update travis.yml to force PHP 5.3.3 to run under Ubuntu Precise #137

Merged
merged 10 commits into from Sep 15, 2017

Conversation

DiegoPino
Copy link
Contributor

@DiegoPino DiegoPino commented Sep 6, 2017

ISLANDORA-2053: (https://jira.duraspace.org/browse/ISLANDORA-2053)
and
ISLANDORA-2035: (https://jira.duraspace.org/browse/ISLANDORA-2035)

http://irclogs.islandora.ca/2017-09-05.html
travis-ci/travis-ci#2963
Islandora/islandora#683

As of September 2017, Travis-CI has removed support for PHP 5.3 (and 5.2) for their Ubuntu Trusty Images, forcing us, in order to keep Drupal's compatibility matrix tested, to run Travis-CI test for 5.3 on Ubuntu Precise

What does this Pull Request do?

This pull Fixes our failing Travis-CI tests:
Should allow PHP 5.3.3 to run under Ubuntu Precise and Higher versions under Ubuntu Trusty.
Changes the way we define which PHP versions and Fedora ones run in Travis.

Also, happens that to be able to fix this under Trusty some of our code fails when using gsto output a TIFF. That is an actual result of newer versions of Ghostscript being more strict about passing invalid combinations of arguments. Like
https://www.ghostscript.com/doc/current/Use.htm#Output_resolution (no ",")
and
https://ghostscript.com/doc/current/Devices.htm#TIFF (no compression for tiff32,64,etc devices)
So had to fix that. That issue was reported in ISLANDORA-2035.

What's new?

We moved PHP 5.3.3 version and the corresponding Fedora versions into the matrix section of the Travis YAML file. Also TIFF generation from PDF should work here and in Vagrant under Trusty again.

How should this be tested?

Travis-CI should pass and everything single PHP/FEDORA version combination should look good and very green.

Interested parties

@Islandora/7-x-1-x-committers

@whikloj
Copy link
Member

whikloj commented Sep 6, 2017

@DiegoPino this one is failing, not sure why. Perhaps because of the new dist: trusty tag?

The problem seems to be Ghostscript.
https://travis-ci.org/Islandora/islandora_paged_content/jobs/272544139#L777
https://travis-ci.org/Islandora/islandora_paged_content/jobs/272544139#L862

@DiegoPino
Copy link
Contributor Author

@whikloj yeah, new Ghostscript release maybe, deprecated arguments?
"Invalid compression setting for this bitdepth".
Did we discuss something like this during committers call? I feel someone brought this up.

Will give it a look. The Charm and elegance of release times 😆

@rosiel
Copy link
Member

rosiel commented Sep 6, 2017

We were just talking about GS versions on the Security interest group - see also Islandora-Labs/islandora_vagrant#127 and https://jira.duraspace.org/browse/ISLANDORA-2035

@DiegoPino
Copy link
Contributor Author

DiegoPino commented Sep 6, 2017 via email

9.10~dfsg-0ubuntu10 is the original (2014)
=9.10~dfsg-0ubuntu10.10 is the security patch (aug 2017)
Maybe there is an issue there. Let's see!
Testing regression to the original Trusty 9.10
@DiegoPino
Copy link
Contributor Author

I'm doing a test.
Trusty got a recent security update to Ghostscript that could, maybe, be the issue here. So I set a fixed version back to the 2014/original one (9.10 also different package) 9.10dfsg-0ubuntu10 v/s 9.10dfsg-0ubuntu10.10
If that does not work i will get the sources and compile. But i really want to avoid that

DiegoPino and others added 5 commits September 6, 2017 16:16
If not done this way you get
he following packages have unmet dependencies:
 ghostscript : Depends: libgs9 (= 9.10~dfsg-0ubuntu10) but 9.10~dfsg-0ubuntu10.10 is to be installed
Again, just a test. Seems like the issue is with a “,” really and
previous versions where less concerned about argument validity
Happens that we need to read the documentation actually.
Old versions of gs where ignoring some bad parameters (like passing
compression to tiff64etc devices). But new ones are actually correct.
To make the case i added a new device and lets see if this works!
@DiegoPino
Copy link
Contributor Author

DiegoPino commented Sep 7, 2017

After N+1 trial and errors solution seems to be (as always) to stay calm, apply flexibility and keep testing.

Issues were:
1.- Old gs was permissive and defaulted in case of wrong arguments for certain devices to good values. But we had some stuff wrong there.
2.- Trusty's gs has a bug(?) or a few. And in case of a device (tiff64nc and tiff48nc) that doesn't allow compression and that we were using with compression, it fails. But there is also a bug, so even passing no compression makes it fail. So many failures.
3. 2.- was fixed in later/recent GS versions, but that fix did not get into Ubuntu's Trusty package
http://git.ghostscript.com/?p=ghostpdl.git;a=commitdiff;h=f8e77523.

So the solution is:
A) make output-to-tiff command building conditional and in specific for tiff64nc and 48nc force -sCompression=none
B) Since that works only if GS is quite recent(odd!) (https://bugs.ghostscript.com/show_bug.cgi?id=696516) I also changed the test to run to something we actually know that works on trusty == device = tiff32nc
C) this needs documentation and/or someone that has more knowledge on GS
D) I feel the compromise is good. Compiling from source to make an invalid test pass seemed to me even more outrageous

PLEASE: squash if merging. There is a lot of trial and error (late night) on this. Travis is actually passing now

// Both are 16bits per channel. No compression possible.
// Was fixed in:
// http://git.ghostscript.com/?p=ghostpdl.git;a=commitdiff;h=f8e77523.
$base_command = '!gs_path -q -dNOPAUSE -dBATCH -sDEVICE=!device -sCompression=none -sOutputFile=!output_file -r!resolution -dFirstPage=!offset -dLastPage=!offset !pdf_uri';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a bigger problem? Can we not compress Tiffs using GS in versions >= 9.10?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can, but only if using tiff32nc or tiff16nc or tiffsep, etc. The issue is that compression actually never ever worked with tiff64nc, it was ignored but it did not fail. 9.10 introduced the issue that running tiff64nc is almost impossible (impossible!) because compression is set to lzw and 16bits per channel simply don´t compress according to tiff 6.0 specs. So that is actually a bug. Same reason we need to document that. If people need to use tiff64nc (you can get the same results using tiffsep and using the don't generate separations flag actually) we need an alternative. One of my previous pulls added actually that alternative to the drop box, but i was afraid i was going to far.
See https://www.ghostscript.com/doc/current/Devices.htm#TIFF
(https://www.ghostscript.com/doc/9.10/Devices.htm#TIFF is actually the same)
The first ones should generate "uncompressed output:" and
-sCompression= is part of tiffsep

In reality, their docs are complex to follow

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also GS in versions 9.10 to 9.15 seems? the rest should work. Anyone willing to test every version?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that seems like overkill. It appears that the fix was first included in the 9.19 release. Should we add that to the note here or just in whatever documentation we do for this?

Copy link
Contributor Author

@DiegoPino DiegoPino Sep 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Go ahead and add a note(comment) in the main conversation (these ones get obscured by changes) and then we can make sure that goes into release as a documentation task. @whikloj++

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should remove the mention of "9.10" in the code note? Based on the git commit date and the various release notes it appears that this bug would affect GS versions 9.10 -> 9.18 inclusive.

@whikloj
Copy link
Member

whikloj commented Sep 7, 2017

Bringing this up a level and suggesting that GhostScript 9.19 is the first version with the fix in their code. That should probably be documented for our users.

@DiegoPino
Copy link
Contributor Author

DiegoPino commented Sep 7, 2017 via email

@whikloj
Copy link
Member

whikloj commented Sep 7, 2017

Right, but as that change is in the main code it could be run on other distributions and is outside the scope of testing only.

@DiegoPino
Copy link
Contributor Author

@whikloj last question: do I remove all comments there or do I leave to mention to the bug? Seems like a way of avoiding to have an open ticket about the same issue in 6 months?
I mean the
// http://git.ghostscript.com/?p=ghostpdl.git;a=commitdiff;h=f8e77523.
with maybe first as context something like
// Under certain Ghostscript versions, this could fail. Fix was committed
Better wording? Remove entirely?

@whikloj
Copy link
Member

whikloj commented Sep 7, 2017

I'd leave the note in and just remove the broken versions or expand it to say

GhostScript versions 9.10 through 9.18 seem to be affected.

or something like that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants