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

feat: use standard Docker build Actions, enable builds for ARM architecture #985

Merged
merged 2 commits into from
Oct 28, 2022
Merged

Conversation

sammcj
Copy link
Contributor

@sammcj sammcj commented Oct 27, 2022

Description

Refactor docker-release (aka build and push) workflow to use the standard Docker Github Actions.

Requirements / Checklist

What does this Pull Request (PR) do?

This adds the following features:

  • Builds for both AMD64 and ARM64 (other architectures can be enabled if required).
  • Automatic tagging and labelling (including 'latest').

How should this be manually tested?

  • I don't have access to your Docker hub credentials so wasn't able to test I could publish as your user, just check it does successfully publish with your creds the first time (but it should be fine!).

Any background context you can provide?

I went to run the nerd-fonts patcher and found the provided container image was only built for AMD64, many modern computers are ARM64 hence this PR.

https://github.com/marketplace/actions/build-and-push-docker-images

What are the relevant tickets (if any)?

Screenshots (if appropriate or helpful)

Copy link
Collaborator

@Finii Finii left a comment

Choose a reason for hiding this comment

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

Thank you for this nice PR.

First I had a deja vu feeling, but that was #882 (comment)

I will just pull as is, and see if the docker is still working on my machine (i.e. amd64).

uses: docker/build-push-action@v3
with:
context: .
push: ${{ github.event_name != 'pull_request' }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
push: ${{ github.event_name != 'pull_request' }}
push: true

Because the whole workflow is not triggered on pull requests.

id: meta
uses: docker/metadata-action@v4
with:
github-token: ${{ secrets.GITHUB_TOKEN }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
github-token: ${{ secrets.GITHUB_TOKEN }}

I think this is not needed.

[why]
Credentials not needed (I guess).
Workflow is never run on pull-requests, so no check needed.

Signed-off-by: Fini Jastrow <[email protected]>
@Finii Finii merged commit 229a154 into ryanoasis:master Oct 28, 2022
@Finii
Copy link
Collaborator

Finii commented Oct 28, 2022

Had a small problem that meta tags it as master but we always tagged as latest.
Solved with 1322285

Tried patching with the docker on amd64 and it works.

@sammcj Can you confirm arm64 please?

(Interestingly the image size did not really increase.)

@Finii
Copy link
Collaborator

Finii commented Oct 28, 2022

Hmm, looking here, I see only amd64 and no arm64 ?

@sammcj Any ideas? I am a docker noob.

@sammcj
Copy link
Contributor Author

sammcj commented Oct 28, 2022

Hey sorry just saw this it's midnight here but I'll fork the repo, create a docker hub account and test it out using my own token. Feel free to temporarily revert if it's causing any issues!

@Finii
Copy link
Collaborator

Finii commented Oct 28, 2022

Ah, I managed to create the arm thing 🎉

image

via commit
df58a78 CI: Entice docker to create arm64 container [skip ci]

But maybe you can try to patch a font with that. And report which fontforge version it has?

@sammcj
Copy link
Contributor Author

sammcj commented Oct 29, 2022

ooohhhh I didn't release it needed the platform there as well! Good spotting and thanks!

I'll be testing it out shortly.

@sammcj
Copy link
Contributor Author

sammcj commented Oct 29, 2022

Seems to work fine, only error is about iconv encoding - but that won't be ARM build related.

I've run the patched fonts through font book and they validated without issue.

  • macOS 13.0
  • Apple Macbook Air (m1)
1.006; ttfautohintv1.8.3;Nerd Fonts 2.3.0-RC
docker run -v /Users/samm/Downloads/Atkinson_Hyperlegible:/in -v /Users/samm/Downloads/Atkinson_Hyperlegible-nerd:/out nerdfonts/patcher
Running with options:

Internal Error: Your version of iconv does not support the "Mac Roman" encoding.
If this causes problems, reconfigure --without-iconv.
Copyright (c) 2000-2022. 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.
 Version: 20220308
 Based on sources from 2022-07-24 20:58 UTC-D.
 Based on source from git with hash: 3714b0323943afc54607f9404b672ca469364cb3
The truetype encoding specified by platform=1 specific=0 (which we map to Mac) is not supported by your version of iconv(3).
Nerd Fonts Patcher v2.3.0-RC (3.2.0) executing

Nerd Fonts: Processing Atkinson Hyperlegible Bold (1/1)
No configfile given, skipping configfile related actions
Adding 177 Glyphs from Seti-UI + Custom Set
╢████████████████████████████████████████╟ 100%
Adding 198 Glyphs from Devicons Set
╢████████████████████████████████████████╟ 100%

Done with Patch Sets, generating font...

Nerd Fonts: Tweaking 1/1
Changing lowestRecPPEM from 8 to 6

Generated: Atkinson Hyperlegible Bold Nerd Font in '/out/Atkinson Hyperlegible Bold Nerd Font.ttf'
Internal Error: Your version of iconv does not support the "Mac Roman" encoding.
If this causes problems, reconfigure --without-iconv.
Copyright (c) 2000-2022. 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.
 Version: 20220308
 Based on sources from 2022-07-24 20:58 UTC-D.
 Based on source from git with hash: 3714b0323943afc54607f9404b672ca469364cb3
The truetype encoding specified by platform=1 specific=0 (which we map to Mac) is not supported by your version of iconv(3).
Nerd Fonts Patcher v2.3.0-RC (3.2.0) executing

Nerd Fonts: Processing Atkinson Hyperlegible Bold Italic (1/1)
No configfile given, skipping configfile related actions
Adding 177 Glyphs from Seti-UI + Custom Set
╢████████████████████████████████████████╟ 100%
Adding 198 Glyphs from Devicons Set
╢████████████████████████████████████████╟ 100%

Done with Patch Sets, generating font...

Nerd Fonts: Tweaking 1/1
Changing lowestRecPPEM from 8 to 6

Generated: Atkinson Hyperlegible Bold Italic Nerd Font in '/out/Atkinson Hyperlegible Bold Italic Nerd Font.ttf'
Internal Error: Your version of iconv does not support the "Mac Roman" encoding.
If this causes problems, reconfigure --without-iconv.
Copyright (c) 2000-2022. 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.
 Version: 20220308
 Based on sources from 2022-07-24 20:58 UTC-D.
 Based on source from git with hash: 3714b0323943afc54607f9404b672ca469364cb3
The truetype encoding specified by platform=1 specific=0 (which we map to Mac) is not supported by your version of iconv(3).
Nerd Fonts Patcher v2.3.0-RC (3.2.0) executing

Nerd Fonts: Processing Atkinson Hyperlegible Italic (1/1)
No configfile given, skipping configfile related actions
Adding 177 Glyphs from Seti-UI + Custom Set
╢████████████████████████████████████████╟ 100%
Adding 198 Glyphs from Devicons Set
╢████████████████████████████████████████╟ 100%

Done with Patch Sets, generating font...

Nerd Fonts: Tweaking 1/1
Changing lowestRecPPEM from 8 to 6

Generated: Atkinson Hyperlegible Italic Nerd Font in '/out/Atkinson Hyperlegible Italic Nerd Font.ttf'
Internal Error: Your version of iconv does not support the "Mac Roman" encoding.
If this causes problems, reconfigure --without-iconv.
Copyright (c) 2000-2022. 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.
 Version: 20220308
 Based on sources from 2022-07-24 20:58 UTC-D.
 Based on source from git with hash: 3714b0323943afc54607f9404b672ca469364cb3
The truetype encoding specified by platform=1 specific=0 (which we map to Mac) is not supported by your version of iconv(3).
Nerd Fonts Patcher v2.3.0-RC (3.2.0) executing

Nerd Fonts: Processing Atkinson Hyperlegible Regular (1/1)
No configfile given, skipping configfile related actions
Adding 177 Glyphs from Seti-UI + Custom Set
╢████████████████████████████████████████╟ 100%
Adding 198 Glyphs from Devicons Set
╢████████████████████████████████████████╟ 100%

Done with Patch Sets, generating font...

Nerd Fonts: Tweaking 1/1
Changing lowestRecPPEM from 8 to 6

Generated: Atkinson Hyperlegible Regular Nerd Font in '/out/Atkinson Hyperlegible Regular Nerd Font.ttf'

@Finii
Copy link
Collaborator

Finii commented Oct 29, 2022

Internal Error: Your version of iconv does not support the "Mac Roman" encoding.

Ah, we base the docker on Alpine iirc, and they use musl's iconv instead of glibc's. And that indeed has no Mac encoding:

image

The relevant code is here: https://github.com/fontforge/fontforge/blob/-/fontforge/encoding.c#L196

LNKLEO pushed a commit to LNKLEO/Nerd that referenced this pull request Nov 24, 2023
CI: Use standard Docker build Actions, enable builds for ARM architecture
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.

2 participants