Bug 231392

Created attachment 197133 [details]
Update print/py-psautohint to 1.8.1

Sorry, I should upload a patch file instead of a shar file.

Created attachment 198711 [details]
Update print/py-psautohint to 1.8.1, version 2

The new patch uses PY_FLAVOR instead of FLAVOR.

Created attachment 200609 [details]
Update print/py-psautohint to 1.9.1

Comment 5 commit-hook 2019-01-31 12:47:15 UTC

A commit references this bug:

Author: swills
Date: Thu Jan 31 12:46:25 UTC 2019
New revision: 491712
URL: https://svnweb.freebsd.org/changeset/ports/491712

Log:
  print/py-psautohint: update to 1.8.1

  PR:		231392
  Submitted by:	Ting-Wei Lan <lantw44@gmail.com> (maintainer)

Changes:
  head/print/py-psautohint/Makefile
  head/print/py-psautohint/distinfo

Comment 6 Steve Wills 2019-01-31 12:48:42 UTC

Committed, thanks!

(In reply to Steve Wills from comment #6)
Please revert the commit. PSAutoHint 1.9.1 depends on ufo feature of print/py-fonttools, and ufo feature depends on devel/py-fs ≥ 2.2.0. It is safe to update PSAutoHint to 1.8.1, but it is not to update it to 1.9.1 until devel/py-fs is updated.

$ psautohint --version
Traceback (most recent call last):
  File "/usr/local/bin/psautohint", line 6, in <module>
    from pkg_resources import load_entry_point
  File "/usr/local/lib/python2.7/site-packages/pkg_resources/__init__.py", line 3191, in <module>
    @_call_aside
  File "/usr/local/lib/python2.7/site-packages/pkg_resources/__init__.py", line 3175, in _call_aside
    f(*args, **kwargs)
  File "/usr/local/lib/python2.7/site-packages/pkg_resources/__init__.py", line 3204, in _initialize_master_working_set
    working_set = WorkingSet._build_master()
  File "/usr/local/lib/python2.7/site-packages/pkg_resources/__init__.py", line 583, in _build_master
    ws.require(__requires__)
  File "/usr/local/lib/python2.7/site-packages/pkg_resources/__init__.py", line 900, in require
    needed = self.resolve(parse_requirements(requirements))
  File "/usr/local/lib/python2.7/site-packages/pkg_resources/__init__.py", line 786, in resolve
    raise DistributionNotFound(req, requirers)
pkg_resources.DistributionNotFound: The 'fs<3,>=2.2.0' distribution was not found and is required by fonttools

Comment 8 Steve Wills 2019-07-03 19:14:40 UTC

Created attachment 205504 [details]
patch to use new py-fonttools-lxml-ufo package as dep

So now that the other port is committed, we can use it as a dep and hopefully have this solved. How does this look to you? I know it creates a conflict, perhaps we can switch other ports to this new package if needed?

Comment 9 Steve Wills 2019-07-03 19:43:39 UTC

Created attachment 205505 [details]
ps-autohint and other py-fonttools deps change

Here's a version that changes the deps for all the ports that I think are related for this. I think this should allow using everything properly and avoid any package conflicts. Does this look right to you?

Created attachment 205520 [details]
Convert fonttools options into metaports

I still prefer converting each fonttools option into a separate package, which I already proposed in https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=237672#c10. It not only makes it easier to track dependencies on optional features but also avoids pulling in unnecessary dependencies when they are not required.

I am not sure whether making these extra ports which install no file is acceptable in ports. I can't get them to pass portlint, but it seems to work fine with portmaster. I haven't tested them in poudriere.

(In reply to Steve Wills from comment #8)
I don't like the way of introducing a slave port conflicting with the master port. I know we can modify all ports depending on py-fonttools to use py-fonttools-lxml-ufo instead, but the problem will occur again once there is a person who adds a new port depending on py-fonttools. Besides, switching all ports to use py-fonttools-lxml-ufo defeats the idea of avoiding unnecessary dependencies, which is the reason why the maintainer of fonttools doesn't want to enable all options by default. Not all ports currently using fonttools require both lxml and ufo features, and we will have to move all ports to another slave port if there is a port requiring an optional feature which isn't enabled in the current slave port.

Comment 12 Po-Chuan Hsieh 2019-07-04 18:32:24 UTC

(In reply to Ting-Wei Lan from comment #10)

I do not have time to test the metaport patch. But I believe it's getting things much more complicated.

Can we go back to my original proposal -- adding the dependencies directly to py-psautohint.
It would look like:

# py-psautohint requires fonttools[lxml]
# See LXML option of print/py-fonttools
RUN_DEPENDS+=   ${PYTHON_PKGNAMEPREFIX}lxml>=4.0<5:devel/py-lxml@${PY_FLAVOR} \
                ${PY_TYPING}
.if ${PYTHON_REL} < 3400
RUN_DEPENDS+=   ${PYTHON_PKGNAMEPREFIX}singledispatch>=3.4.0.3:devel/py-singledispatch@${PY_FLAVOR}
.endif

# py-psautohint requires fonttools[ufo]
# See UFO option of print/py-fonttools
RUN_DEPENDS+=   ${PY_ENUM34} \
                ${PYTHON_PKGNAMEPREFIX}fs2>=2.2.0<3:devel/py-fs2@${PY_FLAVOR}

Though it's slightly different to your idea (psautohint -> fonttools -> lxml/ufo) logically, but it has identical dependencies.
And the comment is clear enough for anyone to check if the dependencies are correct.
At last, fonttools rarely changes the dependencies. That means it won't take much effort to keep it correct and up-to-date.

Comment 13 Steve Wills 2019-07-04 21:35:12 UTC

(In reply to Sunpoet Po-Chuan Hsieh from comment #12)
I don't think we should add the dependencies to py-psautohint. I do not think they belong there since they are used by py-fonttools, not py-psautohint.

(In reply to Sunpoet Po-Chuan Hsieh from comment #12)
psautohint isn't the only port requiring extra features from fonttools. I know it rarely changes, but it doesn't means it won't change. When it changes, it has to be propagated to all ports which copied these dependency lines from fonttools. It should be done in the same commit as the fonttools update to prevent possible breakage. If the person who updates fonttools doesn't know he or she should update copies in other ports as well, these copies may eventually become outdated.

I don't like having duplicate code in a project, but it is OK for me to accept your proposal if you agree to update all ports using extra features from fonttools whenever dependencies of options change. It is not an ideal solution, but I can accept it as long as you don't ask me to keep them in sync by myself and don't leave them outdated. I can add comments to the Makefile of fonttools, so you don't have to search the entire ports tree to find these copies.

The ideal way to resolve the problem is probably adding a USES macro for fonttools, so we will have neither code copies nor many metaports.

Comment 15 Steve Wills 2019-07-06 03:30:37 UTC

(In reply to Ting-Wei Lan from comment #14)
Hmm, I'm not sure fonttools has enough consumers to justify a USES file, but maybe. Could you propose a patch?

Comment 16 Po-Chuan Hsieh 2019-07-07 17:17:33 UTC

(In reply to Ting-Wei Lan from comment #14)

Yes, I'll maintain the fonttools dependencies and update them when necessary.

Comment 17 Steve Wills 2019-07-07 19:38:59 UTC

(In reply to Sunpoet Po-Chuan Hsieh from comment #16)
Please do not add deps to py-psautohint that belong in py-fonttools. IMHO, the deps of a package should be only the things it imports, not transitive dependencies.

Created attachment 205770 [details]
Add a USES=fonttools macro to handle optional features

Comment 19 Steve Wills 2019-07-14 18:17:51 UTC

(In reply to Ting-Wei Lan from comment #18)
Thanks for the proposed patch! I think this doesn't look like the best approach because it moves where we put the incorrect dependency from the ports themselves to the framework.

(In reply to Steve Wills from comment #19)
Yes, but at least it avoids having duplicate code in ports. So we are going back to making a metaport for each fonttools feature?

Created attachment 206886 [details]
Rename py-fonttools-lxml-ufo to py-fonttools-extras and make it install no file

Instead of naming a port with options it enables, such as 'py-fonttools-lxml-ufo', it is better use a generic name, such as 'py-fonttools-extras'. This enables us to change the list of enabled options without changing the name of the port and makes it easier to be used by other ports.

In additional to the name change, I also makes it install no file except for license files, so it won't conflict with the master port. The slave port just pulls in extra dependencies without installing any code of FontTools.

Before applying the patch, you may have to run 'svn rename py-fonttools-lxml-ufo py-fonttools-extras' under /usr/ports/print because svn doesn't seem to provide a way to handle renaming in the patch file.

If attachment 206886 [details] gets accepted, I will update the other 4 ports to use py-fonttools-extras in order to fix fontmake.

Comment 23 Po-Chuan Hsieh 2019-09-02 23:23:41 UTC

(In reply to Ting-Wei Lan from comment #21)

Instead of -extras, I would prefer current naming which is clear enough. And what if other ports require different options?

But I don't like the combination of options. It would be more flexible if a port enables only one option.

Regarding the patch, it looks strange to me. Why not use OPTIONS_EXCLUDE rather than redefining OPTIONS_DEFINE?

What if we move the OPTIONS from Makefile to another file (e.g. options.mk). Let py-fonttools-foo include that file and add py-fonttools and foo_RUN_DEPENDS to RUN_DEPENDS. If a port requires fonttools[foo] and fonttools[bar], you could add py-fonttools-foo and py-fonttools-bar to RUN_DEPENDS.

(In reply to Sunpoet Po-Chuan Hsieh from comment #23)
> Instead of -extras, I would prefer current naming which is clear enough. And
> what if other ports require different options?
When a new option is needed, it will be added to OPTIONS_SLAVE of py-fonttools-extras. The use of -extras is used to avoid renaming and prevent the name from being too long when more options are needed.

> But I don't like the combination of options. It would be more flexible if a
> port enables only one option.
I don't like controlling multiple options with single port, either. I already proposed making one port for each option in attachment 205520 [details], but you didn't seem to review it. Therefore, I just assumed that you had rejected the idea and I had to find a different solution. 

> Regarding the patch, it looks strange to me. Why not use OPTIONS_EXCLUDE
> rather than redefining OPTIONS_DEFINE?
There are 8 options in py-fonttools. In order to hide 'make config' dialog, I have to exclude all options which are not enabled in the slave port, which is a long list that have to be changed every time an option is added to py-fonttools. Redefining OPTIONS_DEFINE means we can just ignore options we don't need, which should be easier to read and maintain.

> What if we move the OPTIONS from Makefile to another file (e.g. options.mk).
> Let py-fonttools-foo include that file and add py-fonttools and
> foo_RUN_DEPENDS to RUN_DEPENDS. If a port requires fonttools[foo] and 
> fonttools[bar], you could add py-fonttools-foo and py-fonttools-bar to 
> RUN_DEPENDS.
That was what I proposed in attachment 205520 [details]. Although I called it 'Makefile.features' instead of 'options.mk', the idea is the same.

(In reply to Sunpoet Po-Chuan Hsieh from comment #23)
Ping ... Can you tell me what should I do here? Should we go back to the approach of attachment 205520 [details] with simplified Makefiles similar to attachment 206886 [details]?

Comment 26 Po-Chuan Hsieh 2019-09-09 19:44:47 UTC

(In reply to Ting-Wei Lan from comment #25)

I was trying to convince you to add ufo to psautohint directly when you submitted attachment 205520 [details]. I still think it's a better solution but I could fallback to multiple ports with -foo suffix which enables foo option. But it would differ from attachment 205520 [details]. I'll see if I could have a patch this weekend.

(In reply to Sunpoet Po-Chuan Hsieh from comment #26)
> I was trying to convince you to add ufo to psautohint directly when you 
> submitted attachment 205520 [details]. I still think it's a better solution
> but I could fallback to multiple ports with -foo suffix which enables
> foo option.
In fact, I thought copy-and-pasting was the worst solution, and I would only consider it when all alternative solutions were rejected. It is nice to know that you considered the metaport approach.

> But it would differ from attachment 205520 [details]. I'll see if I could have
> a patch this weekend.
I think it is fine as long as it doesn't introduce duplicate DEPENDS lines in ports.

(In reply to Sunpoet Po-Chuan Hsieh from comment #26)
Is there any progress on this issue?

Comment 29 Steve Wills 2019-09-25 12:02:54 UTC

print/psautohint should not depend on devel/py-lxml or devel/py-fs2.

Comment 30 Po-Chuan Hsieh 2019-09-25 20:31:47 UTC

(In reply to Ting-Wei Lan from comment #28)

I'm sick since last week. Anyway, here's what I'd like to commit.
https://people.FreeBSD.org/~sunpoet/patch/print-py-fonttools-v2.txt

(In reply to Sunpoet Po-Chuan Hsieh from comment #30)
It doesn't seem to work with Python flavors.

# make install FLAVOR=py27 failed
===>  py27-fonttools-ufo-3.44.0 FLAVOR is defined (to py27) while this port
does not have FLAVORS..
*** Error code 1

Comment 32 Po-Chuan Hsieh 2019-09-27 15:59:40 UTC

(In reply to Ting-Wei Lan from comment #31)

Try https://people.FreeBSD.org/~sunpoet/patch/print-py-fonttools-v3.txt

(In reply to Sunpoet Po-Chuan Hsieh from comment #32)
The new patch works for me.

Created attachment 207906 [details]
Use fonttools features in psautohint and fontmake

This is the patch to unbreak psautohint and fontmake by using fonttools feature metaports proposed by the above patch.

(In reply to Sunpoet Po-Chuan Hsieh from comment #32)
I just saw you committed your fonttools patch into ports. Can you commit attachment 207906 [details] for me?

Comment 36 commit-hook 2019-10-03 18:29:07 UTC

A commit references this bug:

Author: sunpoet
Date: Thu Oct  3 18:28:27 UTC 2019
New revision: 513709
URL: https://svnweb.freebsd.org/changeset/ports/513709

Log:
  Update RUN_DEPENDS: use py-fonttools extra features

  - Bump PORTREVISION for dependency change

  PR:		231392
  Submitted by:	Ting-Wei Lan <lantw44@gmail.com> (maintainer)

Changes:
  head/print/py-psautohint/Makefile
  head/x11-fonts/py-cu2qu/Makefile
  head/x11-fonts/py-defcon/Makefile
  head/x11-fonts/py-fontmake/Makefile
  head/x11-fonts/py-ufo2ft/Makefile

Comment 37 Po-Chuan Hsieh 2019-10-03 18:35:17 UTC

(In reply to Ting-Wei Lan from comment #35)

Committed. Thanks!

Comment 38 Steve Wills 2019-10-03 19:38:06 UTC

Is this issue solved now? Can we close this?

(In reply to Steve Wills from comment #38)
Yes, it is solved and now both psautohint and fontmake work fine for me to build cantarell-fonts v0.111.

I found that all fonttools metaports are removed in ports r513967. Is it an expected change?