Skip to content
This repository has been archived by the owner on Jul 4, 2023. It is now read-only.

fontforge update for python #31226

Closed
wants to merge 7 commits into from

Conversation

monkeyiq
Copy link
Contributor

I found that configure didn't find the python.pc files using pkg-config without explicitly updating the PKG_CONFIG_PATH from the brew file. Sorry if this the incorrect way of going about this, no amount of adding depends_on 'python' was letting brew include python into the PKG_CONFIG_PATH.

without explicitly updating the PKG_CONFIG_PATH from the brew file. Sorry
if this the incorrect way of going about this, no amount of adding
depends_on 'python' was letting brew include python into the PKG_CONFIG_PATH.
@adamv
Copy link
Contributor

adamv commented Jul 30, 2014

@BrewTestBot test this please

@mistydemeo
Copy link
Member

Is python mandatory for the HEAD build?

@monkeyiq
Copy link
Contributor Author

Yes, git master of fontforge now has a mandatory dependency on python:
https://github.com/fontforge/fontforge/blob/master/configure.ac#L209

@@ -41,7 +41,11 @@ class Fontforge < Formula
option 'with-x', 'Build with X11 support, including FontForge.app'

depends_on 'gettext'
depends_on :python => :optional
if build.head?
depends_on 'python'
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand this; what's the actual dependency? Python is provided by OSX; is the system version insufficient?

Copy link
Contributor

Choose a reason for hiding this comment

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

Without homebrew's python, will fontforge's python module work?

Copy link
Contributor

Choose a reason for hiding this comment

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

I checked on a 10.8 machine and got this:

==> ./configure --prefix=/usr/local/Cellar/fontforge/HEAD --enable-double --with
checking for update-mime-database... :
checking for update-desktop-database... :
checking for plutil... /usr/bin/plutil
checking whether /usr/local/bin/python version is >= 2.7... no
configure: error: Python interpreter is too old

READ THIS: https://github.com/Homebrew/homebrew/wiki/troubleshooting

These open issues may also help:
fontforge update for python (https://github.com/Homebrew/homebrew/pull/31226)
fontforge: Update formula for 2.0 (https://github.com/Homebrew/homebrew/pull/27221)

Copy link
Member

Choose a reason for hiding this comment

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

It shouldn't be looking at /usr/local/python and any Python version installed there should be 2.7 anyway? Seems we need to debug their buildsystem a bit here. For what it's worth depends_on :python ensures 2.7 is being used anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The failing test is the call to AM_PATH_PYTHON([2.7])
https://github.com/fontforge/fontforge/blob/master/configure.ac#L209

For me, on osx 10.8 the above test fails to find python due to pkg-config failing to find it. I should mention that I am not experienced with brew, and apologise in advance if there is something I should be doing to have pkg-config find python which I am not doing :(

One reason I am looking at the python from homebrew is that I can get a known version that way. For example on osx 10.6 for which there are many users who are interested in HEAD fontforge, and sufficiently so that they might like to brew.

AFAIK the dependency is on python 2.7 able to be found through pkg-config. Though I have no experience testing the fontforge python module against the system python.

Copy link
Member

Choose a reason for hiding this comment

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

:python gives you a known version: 2.7 or above. Please use that.

@davelab6
Copy link
Contributor

davelab6 commented Aug 1, 2014

@monkeyiq It works! :) But I got 3 warnings:

$ brew install fontforge --HEAD --with-x
...
==> ./configure --prefix=/usr/local/Cellar/fontforge/HEAD --enable-double --with
Warning: inreplace: replacement of '/Applications' with '$(prefix)' failed
Warning: inreplace: replacement of 'ln -s /usr/local/bin/fontforge' with 'ln -s $(bindir)/fontforge' failed
Warning: inreplace: replacement of 'python setup.py install --prefix=$(prefix) --root=$(DESTDIR)' with 'python setup.py install --prefix=$(prefix)' failed
==> make

I also noticed that after installing it, the 'based on source with git hash' identifier doesn't work:

$ brew install python;
$ brew install fontforge --HEAD --with-x;
$ fontforge;
Copyright (c) 2000-2014 by George Williams. See AUTHORS for Contributors.
 License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
 with many parts BSD <http://fontforge.org/license.html>. Please read LICENSE.
 Based on sources from 02:14 EDT  1-Aug-2014-ML-D.
 Based on source from git with hash:
$

Also this failed to build with the zeromq collab feature which should no longer be optional :(

@davelab6
Copy link
Contributor

davelab6 commented Aug 1, 2014

Collab still didn't work with:

brew install czmq zmq;
brew install fontforge --HEAD --with-x;

@monkeyiq
Copy link
Contributor Author

monkeyiq commented Aug 2, 2014

@dave I hadn't started on making collab go yet. I was hoping to make the first PR as small as possible so that we got 'a build' going again and make the second PR contain the fixes that are needed for collab to work.

I might start on the collab changes soon in parallel to this so that travis ci osx can start building and with collab enabled. Then feed the collab changes in as a separate PR to homebrew :)

@monkeyiq
Copy link
Contributor Author

monkeyiq commented Aug 2, 2014

@dave the git hash not working is because brew makes a shallow copy of the git tree, sans the .git directory, which is where the source is then configured and bootstrapped.

So this block fails during the ./configure
https://github.com/fontforge/fontforge/blob/master/configure.ac#L720

A few options I might explore are to either pass the original source tree location or the git hash explicitly to ./configure during build. fwiw port also builds in a .gitless copy so both places would benefit from such an option.

@monkeyiq
Copy link
Contributor Author

monkeyiq commented Aug 2, 2014

As with fontforge built using macports, you can run it from /usr/local/bin or /opt/local/bin but that will likely not use the "FontForge" script and thus not use the inclusive stuff like themes etc. Much of what FontForge does, and the files it points to depend on being in /Applications/FontForge.app (fontconfig handling being one example).

@davelab6
Copy link
Contributor

davelab6 commented Aug 2, 2014

@monkeyiq ah sorry I thought you said you'd do them at once... :) No
worries, collab can wait - since no one can brew fontforge head at the
moment, and FF is going to make a new stable release in the next week or
two, I agree getting it fixed first and iterating would be great :)

shell script into the python program itself to simplify.
@monkeyiq
Copy link
Contributor Author

monkeyiq commented Aug 2, 2014

@MikeMcQuaid Update committed. I now use an EOS structure to contain the python program. The shell script line to get the absolute path is rolled into the python code. The puts is removed.

I have tested this new version by replacing my local fontforge.rb with the one from this PR and:

brew remove fontforge;
brew install --verbose --debug fontforge --HEAD --with-x

@@ -35,13 +35,19 @@ class Fontforge < Formula
depends_on 'pango'
depends_on 'cairo'
depends_on 'ossp-uuid'
depends_on 'zeromq'
depends_on 'czmq'
Copy link
Member

Choose a reason for hiding this comment

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

Are either of these optional?

Copy link
Member

Choose a reason for hiding this comment

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

Or only required for head?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed them to be => optional.

Copy link
Contributor

Choose a reason for hiding this comment

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

Collab is now required.
On 2 Aug 2014 11:42, "Mike McQuaid" [email protected] wrote:

In Library/Formula/fontforge.rb:

@@ -35,13 +35,19 @@ class Fontforge < Formula
depends_on 'pango'
depends_on 'cairo'
depends_on 'ossp-uuid'

  • depends_on 'zeromq'
  • depends_on 'czmq'

Are either of these optional?


Reply to this email directly or view it on GitHub
https://github.com/Homebrew/homebrew/pull/31226/files#r15729572.

@monkeyiq
Copy link
Contributor Author

monkeyiq commented Aug 3, 2014

moved to using EOS.undent.

@davelab6
Copy link
Contributor

davelab6 commented Aug 6, 2014

should this pull request be closed and a new fresh one made?

@monkeyiq
Copy link
Contributor Author

monkeyiq commented Aug 6, 2014

@davelab6 I am happy to make a new PR if it will help with merging.

In general I would also be happy to maintain the fontforge.rb for head builds. I suspect that there isn't fine grained enough ACL on github, but I'd be happy to make an informal aggrement that I'd only merge PRs that touch fontforge.rb. I would concentrate only on the head build, but other changes might be nice for the rest of the file too.

I also use a Formula which is an extension of this for building fontforge in Travis CI:
https://github.com/fontforge/fontforge/blob/master/travis-scripts/fontforge.rb

@davelab6
Copy link
Contributor

davelab6 commented Aug 6, 2014

The big yellow 'failed' sign below suggests something still isn't right, but your Forumla works perfectly for me, and, thanks to your https://github.com/fontforge/fontforge/blob/master/travis-scripts/before_install_osx.sh#L7 we are now always testing that homebrew can build the git master head and its passing all the time now :)

I think that Travis check means that if someone sends a PR the breaks homebrew, they will have to fix https://github.com/fontforge/fontforge/blob/master/travis-scripts/fontforge.rb in order to get travis to pass their PR.

@monkeyiq
Copy link
Contributor Author

Created a new PR to clean things up and try to remove the "Failed" yellow link below:
#31500

@monkeyiq
Copy link
Contributor Author

#31500

@monkeyiq monkeyiq closed this Aug 10, 2014
@Homebrew Homebrew locked and limited conversation to collaborators Feb 17, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants