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

Re-implementing rvrn feature #67

Merged
merged 3 commits into from
Jan 2, 2022

Conversation

aaronbell
Copy link
Contributor

Discovered that in the original build script, the rvrn feature was being implemented separately from the general build process. So once the build process switched to UFR, the rvrn feature no longer worked.

To resolve this issue, I've re-integrated the .rvrn glyphs back into the base variants using the standard Glyphs bracket trick, and updated the features to reflect the new export naming convention.

Verified that the feature (as well as titl and tnum) are working once again.

I made an error in my previous PR in disabling autohinting for the static instances. This PR corrects that. Fonts rebuilt.
Changing to the UFR format resulted in the rvrn feature no longer working.

The rvrn variants have been reimplemented using the Glyphs bracket trick (which fontmake can process), and features updated to work correctly.

rvrn variants have thus been removed from the font sources.
@aaronbell
Copy link
Contributor Author

@weiweihuanghuang When you have a chance, can you take a look at this?

@weiweihuanghuang
Copy link
Owner

Hey Aaron I haven't had a chance to look yet but I just saw this issue, do you know anything about this?
#70

@aaronbell
Copy link
Contributor Author

I don’t. It appears that the actions, here at least, completed successfully.

@bennetfabian
Copy link

I don’t. It appears that the actions, here at least, completed successfully.

The last run was a few months ago. When you run it again right now it fails.

@bennetfabian
Copy link

This is what running this action now looks like:
https://github.com/bennetfabian/Work-Sans/actions/runs/1604928810

@aaronbell
Copy link
Contributor Author

Looks like there is an issue with processing a particular glyph layer name:

File "/home/runner/work/Work-Sans/Work-Sans/venv/lib/python3.8/site-packages/ufoLib2/objects/layerSet.py", line 227, in getitem
224
layer_object = self._layers[name]
225
KeyError: '{137}'

It may be that ufoLib2 doesn’t support brace layers. And it may be that the newer version of ufoLib2 has resolved that limitation (which is why upgrading appears to fix things).

Not sure why the issue would crop up on the forked repro and not here, though.

@bennetfabian
Copy link

Looks like there is an issue with processing a particular glyph layer name:

File "/home/runner/work/Work-Sans/Work-Sans/venv/lib/python3.8/site-packages/ufoLib2/objects/layerSet.py", line 227, in getitem

224

layer_object = self._layers[name]

225

KeyError: '{137}'

It may be that ufoLib2 doesn’t support brace layers. And it may be that the newer version of ufoLib2 has resolved that limitation (which is why upgrading appears to fix things).

Not sure why the issue would crop up on the forked repro and not here, though.

Well, the GitHub Action always pulls the latest version of all libraries so the Action run from 5 months ago ran with totally different versions as they were the latest back then but not anymore.

Maybe we could change the action's "on" parameter and add workflow_dispatch so it can be ran manually. I bet that it will output the same if we run it in this repo.

@bennetfabian
Copy link

bennetfabian commented Dec 23, 2021

Yup, just confirmed that back then on August 6 when the action last ran the latest versions of the packages were:
fontmake==2.4.0
gftools==0.7.4

My common sense suggests that the updates to those packages in the meantime since Aug 6 somehow broke the building process.

Now the question is if we

  • just do the comfortable but maybe riskier way of changing the requirements.txt to use the latest version which doesn't fail
  • try to fix the sources itself which may be more future-proof if there ever are any updates to the repo again or OpenType specs get changed

What do you guys think? @weiweihuanghuang @aaronbell

@aaronbell
Copy link
Contributor Author

I would suggest changing the sources. 🙂

@weiweihuanghuang
Copy link
Owner

Thanks for the rundown. @aaronbell In what way should I change the source?

@bennetfabian
Copy link

I can report that Work Sans now builds successfully without any changes. The new updates in the underlying libraries used for font building must have been updated to fix the problem!
Therefore #70 can be closed.

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

Successfully merging this pull request may close these issues.

3 participants