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

Changed font creation process #1413

Merged
merged 20 commits into from
Sep 13, 2023
Merged

Conversation

schmidt-oliver
Copy link
Contributor

@schmidt-oliver schmidt-oliver commented Jun 29, 2023

Moved font creation to node library svgtofont.

Fixes these Issues:

Because of a problem with path directions in outline-stroke (see #5) I moved the creation of outline SVGs to svg-fixer.

There are some options which could be changed in both build scripts. Please test yourself and change them if needed. Tried to adapt your code style. Updated the github workflow. I hope I didn't missed something.

New process:

  • Build outline icons with svg-fixer
  • Build font with svgtofont
  • Do not delete the created ./lucide-font folder. The info.json file that is created the first time contains the code points that will be used in the next execution. Existing code points will be reused. New Icons will be added after existing.

Tested the whole process myself. But you should test it yourself before bringing it to production.

Copy link
Member

@ericfennis ericfennis left a comment

Choose a reason for hiding this comment

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

Fantastic job, I'm also impressed with the amount of output and different files it has for using the font. And JSON file is really useful for the site.
I noticed that the CSS classes are now prefixed with lucide- instead of icon-, maybe we should keep the old class prefix for backward compatibility.

The lucide-font.yml workflow has currently container option declared for the ruby environment but if we switch to this flow we can remove that one.

Class name prefix added for backward compatibility.
Removed the container option in github workflow lucide-font because it is not needed anymore, workflow was changed to nodejs only
@schmidt-oliver
Copy link
Contributor Author

Thank you for pointing this out. Fixed both.

  • I noticed that the CSS classes are now prefixed with lucide- instead of icon-, maybe we should keep the old class prefix for backward compatibility.
  • The lucide-font.yml workflow has currently container option declared for the ruby environment but if we switch to this flow we can remove that one.

@ericfennis
Copy link
Member

@schmidt-oliver Thanks for your changes, this looks really nice!
One thing I'm not sure about and that the info.json file. When releasing a new version the workflow need to have this file to know the old assigned codepoints.
So I thought maybe we can think of something to define those unicodes without the need of the info.json file. And the best way I think is counting up the unicode number from when the icon was added to the library.

So for the website we already fetching this data with scripts/writeReleaseMetaData.mjs. This outputs a file with these contents:

{
  "accessibility": {
    "createdRelease": {
      "version": "0.33.0",
      "date": "2022-04-24T12:12:44+02:00"
    },
    "changedRelease": ...
  },
  "activity-square": {
    "createdRelease": {
      "version": "0.244.0",
      "date": "2023-06-12T21:36:07+02:00"
    },
    "changedRelease": ...
  },
...

We could use createdRelease to sort the whole list of icons and then use these order to define the unicodes:
So if we start counting from  we can count up all icons indexes up to this unicode number. This way we don't need to share this info.json between builds. And another thing is that we probably can use this really easy for the website as well. (code examples, copy character code, etc)

Let me know what you think of this idea

@schmidt-oliver
Copy link
Contributor Author

schmidt-oliver commented Jul 7, 2023

Interesting idea. You can fetch the json file, sort the list by createdRelease.date and add the index of the icon to the unicode start number. Sounds great, but the writeReleaseMetaData.mjs does not contain modules to import. Should we assume that the json file releaseMetaData.json already exists in .vitepress/data while creating the font? If so, I can adjust the process accordingly.

@ericfennis
Copy link
Member

@schmidt-oliver Yeah, the easiest way is simply to run the scripts/writeReleaseMetaData.mjs before we run the build font script. So this way you can import the JSON file and do the mapping in your script.

@schmidt-oliver
Copy link
Contributor Author

Well, I will probably update the build script within the next week.

@ericfennis
Copy link
Member

@schmidt-oliver yeah, take your time. Thanks for the work you already did!

@schmidt-oliver
Copy link
Contributor Author

@ericfennis thank you.

I think this approach only works if the release information of all icons is found, but I got this warning message after running pnpm --filter docs prebuild:releaseJson:

Could not find release metadata for icon 'check'.
Could not find release metadata for icon 'circle'.
Could not find release metadata for icon 'clock'.
Could not find release metadata for icon 'code'.
Could not find release metadata for icon 'compass'.
Could not find release metadata for icon 'fast-forward'.
Could not find release metadata for icon 'filter'.
Could not find release metadata for icon 'layers'.
Could not find release metadata for icon 'more-horizontal'.
Could not find release metadata for icon 'more-vertical'.
Could not find release metadata for icon 'navigation-2'.
Could not find release metadata for icon 'navigation'.
Could not find release metadata for icon 'octagon'.
Could not find release metadata for icon 'play-circle'.
Could not find release metadata for icon 'play'.
Could not find release metadata for icon 'rewind'.
Could not find release metadata for icon 'shield'.
Could not find release metadata for icon 'star'.
Could not find release metadata for icon 'target'.
Could not find release metadata for icon 'volume'.
Could not find release metadata for icon 'zap'.

I found out, that the createdRelease.version of the icons above is set to 0.0.0, but a createdRelease.date does exist. So, is it save to ignore the warning message above? And is it ensured that the createdRelease.date is always set and won't change?

If some icons have the same createdRelease.date i would sort them alphabetically. That should work fine.

@ericfennis
Copy link
Member

Yeah you can ignore these warnings. These icons are created before the fork (forking from feather). They should have the date of the fork.
I think if multiple icons have the same date, then they should be sorted by name.

@schmidt-oliver
Copy link
Contributor Author

Hey @ericfennis, found two icons where icon.releaseDate is missing: ban and panel-left. Can you please check why and fix that issue?

@ericfennis
Copy link
Member

@schmidt-oliver I Found the issue, fixed it on the main branch, and rebased this PR.
Can you update the pnpm-lock.yaml?

@github-actions github-actions bot added the 📦 dependencies Pull requests that update a dependency file label Jul 24, 2023
@schmidt-oliver
Copy link
Contributor Author

@ericfennis I have changed the process from using the info.json file to using the version information. I set startUnicode = 57400; as you suggested. Since you fixed the missing information of the two icons, everything works fine. I've checked the sort order twice to make sure it's correct. The code is linted.

I have a suggestion regarding the releaseMetaData.json file. My function convertReleaseMetaData converts the content of this file to the following notation (array, name as attribute) and adds the index according to the defined sort order and the unicode according to the startUnicode and index.

[
  {
    createdRelease: { version: '0.0.0', date: '2020-06-08T16:39:52+0100' },
    changedRelease: { version: '0.244.0', date: '2023-06-12T21:36:07+02:00' },
    name: 'activity',
    index: 0,
    unicode: 57400
  },
  {
    createdRelease: { version: '0.0.0', date: '2020-06-08T16:39:52+0100' },
    changedRelease: { version: '0.18.0', date: '2022-04-06T11:13:36+02:00' },
    name: 'airplay',
    index: 1,
    unicode: 57401
  },
  {
    createdRelease: { version: '0.1.0-alpha.0', date: '2020-06-08T16:39:52+0100' },
    changedRelease: { version: '0.152.0', date: '2023-04-16T14:17:17+02:00' },
    name: 'alarm-clock',
    index: 2,
    unicode: 57402
  },
  {
    createdRelease: { version: '0.11.0', date: '2020-06-08T16:39:52+0100' },
    changedRelease: { version: '0.133.0', date: '2023-04-09T11:46:30+02:00' },
    name: 'album',
    index: 3,
    unicode: 57403
  },
  // ...
]

As we want to display the Unicode numbers on the website, it would be smart to modify the data directly where it is created. After that we could use the prepared data for the font creation. What do you think about that?

Copy link
Member

@ericfennis ericfennis left a comment

Choose a reason for hiding this comment

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

@schmidt-oliver Fantastic work. Looks really good.
I don't fully understand the last part of your last comment, do you mean to run the website builds first and then use the created "data" for the font builds?

tools/build-font/main.mjs Outdated Show resolved Hide resolved
@schmidt-oliver
Copy link
Contributor Author

schmidt-oliver commented Jul 25, 2023

@ericfennis Yes, that's more or less how I imagined it. Maybe in a separate step before generating the website as well as the font. For example, with a data structure like this:

{
  meta: {
    name: 'lucide',
    version: '0.263.0',
    iconsTotal: 1215,
  },
  data: [
    {
      name: 'activity',
      tags: ['...'],
      categories: ['...'],
      contributors: ['...'],
      index: 0,
      unicode: 57400,
      createdRelease: { version: '0.0.0', date: '2020-06-08T16:39:52+0100' },
      changedRelease: { version: '0.244.0', date: '2023-06-12T21:36:07+02:00' },
      // ...
    },
    // ...
  ]
}

@schmidt-oliver
Copy link
Contributor Author

@victorbnl I only had to fix the pnpm-lock.yaml conflict again.
@ericfennis Project is ready to use, only the merge is missing. Hope this time it will be merged before any other conflicts occur ^^. Would be nice if it gets merged soon. I also want to use it in production without my own fork.

Have a nice day =)

@ericfennis
Copy link
Member

@schmidt-oliver Awesome! I will review and test this.
But let's get this one merged this week!

@lucide-icons lucide-icons deleted a comment from github-actions bot Sep 11, 2023
@ericfennis ericfennis self-assigned this Sep 11, 2023
@schmidt-oliver
Copy link
Contributor Author

@ericfennis Awesome, I re-checked it today and it's working well on my local machine. If any issues arise in the future, I will try to resolve them.

@victorbnl, you are welcome to review it as well. The project installation is based on pnpm. After package installation, two scripts are required to generate the font: build:outline-icons and build:font.

  • The build:outline-icons script uses oslllo-svg-fixer to convert the icons to their outline version. This is necessary for TTF conversion. The previously used library, outline-stroke, did not work as intended and had to be replaced.

  • The build:font script uses svgtofont to transform the outlined version of the icons into a font. It takes the release metadata of each icon and its name to order them. Based on that sort order the unicode code points are created. If svgtofont becomes unsupported in the future, fantasticon could serve as an alternate solution, since both tools rely on the same dependencies.

I thought a little summary would be a good idea.

@victorbnl
Copy link

Nice, thank you for the summary! Looks quite good to me, but two things:

  • Why use two different scripts for build:outline-icons and build:font? As far as I know build:outline-icons is only used in order to use build:font after, and build:font requires build:outline-icons. Thus separating them just makes an additional unnecessary script one has to run for no reason.

  • Are you planning to automate the font build and publish it somewhere, say for each version as a GitHub release? That would be very nice as well.

@schmidt-oliver
Copy link
Contributor Author

Hi @victorbnl,

  • Are you planning to automate the font build and publish it somewhere, say for each version as a GitHub release? That would be very nice as well.

- name: Install dependencies
run: pnpm install --filter outline-svg
- name: Outline svg Icons
run: pnpm build:outline-icons
- name: Install dependencies
run: pnpm install --filter build-font
- name: Create font in ./lucide-font
run: pnpm build:font

Both steps are part of the GitHub workflow and the final font is released in each package. They are not intended to be run by an end user.

  • Why use two different scripts for build:outline-icons and build:font? As far as I know build:outline-icons is only used in order to use build:font after, and build:font requires build:outline-icons. Thus separating them just makes an additional unnecessary script one has to run for no reason.

lucide/package.json

Lines 18 to 19 in 12d612d

"build:outline-icons": "pnpm --filter outline-svg start",
"build:font": "pnpm --filter docs prebuild:releaseJson && pnpm --filter build-font start",

Each script does it's own job and as long as they aren't intended to be run by an end user it's totally fine to separate them in my opinion. This way it is a bit clearer and easier to identify problems in the code and to write tests. But sure, it's also possible to combine all the steps in a single task:

 "build:font": "pnpm --filter outline-svg start && pnpm --filter docs prebuild:releaseJson && pnpm --filter build-font start", 

@ericfennis
Copy link
Member

@schmidt-oliver Awesome work. I tested this with an old HTML file from the old font and it all looks great.

1 change I'm not totally convinced yet is chang in the outline-icons step. Previously Github Actions took ~1m 45s to complete, with the changes in this PR the build time will increase to +6min (according to the Lucide font checks workflow). To me this is a bit too long, Can you explain why we should move forward with the new workflow?

@schmidt-oliver
Copy link
Contributor Author

schmidt-oliver commented Sep 13, 2023

@ericfennis Thank you.

1 change I'm not totally convinced yet is chang in the outline-icons step. Previously Github Actions took ~1m 45s to complete, with the changes in this PR the build time will increase to +6min (according to the Lucide font checks workflow). To me this is a bit too long, Can you explain why we should move forward with the new workflow?

  • The build:outline-icons script uses oslllo-svg-fixer to convert the icons to their outline version. This is necessary for TTF conversion. The previously used library, outline-stroke, did not work as intended and had to be replaced.

No problem, the reason is described in this issue. But I will try to break it down. The problem is based on the way how the outlines are created. The outline paths can be clockwise or counter-clockwise. And there are rules to determine whether the paths are filled or cut out. The even-odd rule is used in the outline-stroke library and is not compatible with the svgtofont library and its dependencies svgicons2svgfont, svg2ttf. To create the font with these libraries, which are the only JS-based libraries I've found, you need to have outlined icons based on the older non-zero rule. The oslllo-svg-fixer takes this rule into account and respects the path directions. It seems that outlining with this rule takes more time. It's possible to reduce the time by reducing the precision of the outlining process. But I don't recommend it.

More information about the the two rules:

Can you explain why we should move forward with the new workflow?

The old workflow is not based on JS and we have some improvements (bugfixes and features) as mentioned above. The outlining process takes about 0.3 seconds per icon, depending on its complexity and hardware performance. The build time will increase with the number of icons. I think it's fine.

@victorbnl
Copy link

victorbnl commented Sep 13, 2023

Both steps are part of the GitHub workflow and the final font is released in each package. They are not intended to be run by an end user.

My bad, as I saw there was no action running on your fork I thought you hadn’t made one. Apparently you just didn’t enable them. That’s nice!

Each script does it's own job and as long as they aren't intended to be run by an end user it's totally fine to separate them in my opinion. This way it is a bit clearer and easier to identify problems in the code and to write tests. But sure, it's also possible to combine all the steps in a single task

Given it’s run in an action, it doesn’t matter much indeed.

@victorbnl
Copy link

victorbnl commented Sep 13, 2023

Why run the workflow on pull requests though? The font should be built every time Lucide icons change, right?

Also, but this is only a suggestion, I think it would make sense to make a new release with the font on version bumps. It would make it possible more easily to get the font of the latest release (as opposed to that of the latest commit), for more stability.

For context, I’ll make an Arch package for the font and it would be nice to have two packages: ttf-lucide-icons-git (development) and ttf-lucide-icons (stable releases). I think the latter would be harder to make with only artifacts on every commit.

@ericfennis
Copy link
Member

@schmidt-oliver Thanks makes sense! The process probably can speed up if the processes are done in worker pools. Looks like SVGFixer doesn't do that yet. Could be a nice improvement.

But that's aside I think it's ready to go then!

@victorbnl As a maintainer I like to have workflows run as fast as possible. During a release, the font workflow is dependent on two other workflows that build packages. So that's why I'm not happy with this increase in time. But I will see if we can speed things up later.

Also, the Font workflow is not running on all PRs, only if there are changes in: icons, font build scripts, and the package-lock file. Why? Because then we can know if the font process is still working correctly or if something has changed.

@ericfennis ericfennis merged commit b8c3a5f into lucide-icons:main Sep 13, 2023
12 checks passed
@victorbnl
Copy link

@victorbnl As a maintainer I like to have workflows run as fast as possible. During a release, the font workflow is dependent on two other workflows that build packages. So that's why I'm not happy with this increase in time. But I will see if we can speed things up later.

I didn’t discuss the process speed, was it me you wanted to mention?

Also, the Font workflow is not running on all PRs, only if there are changes in: icons, font build scripts, and the package-lock file. Why? Because then we can know if the font process is still working correctly or if something has changed.

Well from what I understand the font created in this action is not directed towards end users but Lucide maintainers for testing then. In that case, where is the user who wants to use the font supposed to get it from?

@schmidt-oliver
Copy link
Contributor Author

@schmidt-oliver Thanks makes sense! The process probably can speed up if the processes are done in worker pools. Looks like SVGFixer doesn't do that yet. Could be a nice improvement.

Interesting idea. I have never worked with worker pools before. The oslllo-svg-fixer allows a single SVG as parameter instead of a folder path. So it's possible to handle the worker pools without touching the library. Let me see if I can do something about that.

@ericfennis
Copy link
Member

@schmidt-oliver I'm already working on it.
The first tests are really promising.

See my PR: oslllo/svg-fixer#89

@danielbayley
Copy link
Member

@ericfennis @schmidt-oliver @victorbnl Do we need to run the entire set of SVGs through the svg-fixer outline step on every build? I wonder if we could cache the intermediate outlined SVGs, and only rerun the outline step over those that have changed since the last build…?

@schmidt-oliver
Copy link
Contributor Author

@danielbayley Sure, we could reuse them. Then we need to store the date or version number of each outlined icon during the outline step and compare it to its changedRelease.date or changedRelease.version. If the release is newer, we need to rebuild it.

@Finii
Copy link

Finii commented Oct 19, 2023

@ericfennis wrote here #1413 (comment):

One thing I'm not sure about and that the info.json file. When releasing a new version the workflow need to have this file to know the old assigned codepoints.

I just wanted to note that the info.json approach was far superior and did in fact solve #514. The new approach does shift all codepoints if an old icon is dropped as the vacant slot can not be left empty and/or be filled with a new glyph. If a icon is renamed the confusion will be more local possibly shifting only some icons, but still.

Whatever the problem is with also fetching the info.json file and committing it back on icon additions, I seem to not get it. Maybe it would be worthwhile to solve the problems that Eric saw with that file, to really solve 514. Other icons collections do it that way, its not that it would be impossible? The now deserted but in former times often used svg-to-font generator FontCustom automagically did it...

Edit: Fix typos

@ericfennis
Copy link
Member

@Finii So you're telling us that #514. Is not fixed? That's not what we wanted.
Unfortunately info.json approach is not feasible. That would mean we need to keep that file constantly updated when new icons are added, and that is just too much work. And automatically pushing it to the main is not possible, due to security reasons (main is a protected branch).

But It is a bummer to hear that this is still not working correctly as we expected.
Can you create an issue for this?

@danielbayley
Copy link
Member

@ericfennis This might be a half baked idea—I’m stretched too thin 😅—But could the general approach not be to save the associated codepoint of each icon as simply another property of it’s existing accompanying metadata file? Then in whatever post-merge automation, make use of ??= assignment, so that no existing codepoint gets overwritten… But should an icon be removed—naturally along with its metadata—then a ‘slot’ would naturally open up in this loop, allowing another (only newly added) icon to fill that codepoint? cc: @schmidt-oliver

@ericfennis
Copy link
Member

@danielbayley Yeah I thought of this idea as well. This great solution for icons that will be renamed. But the problem is what to do with new icon PRs. If we have multiple PRs open which icon will get which unicode. If some other PR is merged the other one need to update it. So that's not very practical. And you can't simply just fill in something. Unicode positions are limited. 😞

@danielbayley
Copy link
Member

the problem is what to do with new icon PRs. If we have multiple PRs open which icon will get which unicode. If some other PR is merged the other one need to update it.

@ericfennis Would this not cease to be a problem if we could make sure every PR was first rebased onto the last merged? New icons would simply take up next available slots, no?

Unicode positions are limited.

Isn’t the limit something like ~150,000 though? Not even I am going to churn out that many icons… 😅

@Finii
Copy link

Finii commented Oct 19, 2023

Just as idea ...

That would mean we need to keep that file constantly updated when new icons are added, and that is just too much work. And automatically pushing it to the main is not possible, due to security reasons (main is a protected branch).

  • A new icon does not get any codepoint associated in the PR (as it is impossible to say which it would get, as you pointed out)
  • A workflow is triggered on changes in the svg directory just on pulls to master/main. That workflow checks all svgs that are existing in that workflow and adds new icons to the icons.json at the bottom (i.e. next free codepoint).
  • If the icons.json is changed by the workflow the workflow itself commits that changes to master/main (in fact you can always commit it back, if there is no change nothing will happen anyhow)
  • If an existing icons is removed or renamed the PR for that needs to change the icons.json accordingly

That means almost no manual attention is needed.

And I can not see the security reasons you mention. The workflow that is triggered on pulls to master can only be triggered by persons/processes that are allowed to pull to master. The workflow inherits that right, and can add one more commit on the fly.

At Nerd Fonts for example we

  • on pushes to master that change any relevant files
  • we create a zip file from all files that are needed for custom patching
  • commit the new version of the zip back automatically

See https://github.com/ryanoasis/nerd-fonts/blob/master/.github/workflows/zip-release.yml

Edit: Add link to NF workflow

@ericfennis
Copy link
Member

@Finii We had something similar before, it was a workflow that updated our packages.json files after our releases in our GitHub repo.
That used the GitHub action token to push commits to our main branch. That worked fine for a long time. But we hadn't turned on branch protection rules on our main branch. We realized this was needed to secure our project, but that made it impossible to push back from GitHub actions because Github Actions is not a user you can give access to your branch. See https://github.com/orgs/community/discussions/13836
And I'm not in favor of using PAT tokens, because that is a big possible security risk.

If you are doing it with the use of branch protection rules and without PATs, I really want to know how. I've searched a lot Internet for a solution but never found a solution.

@schmidt-oliver
Copy link
Contributor Author

@Finii @ericfennis @danielbayley: We are creating a releaseJson file that contains all the information about icon releases and changes, including their timestamps. If it is possible to find out when an icon was deleted or renamed (a renamed icon could be handled like a deleted and a new one), it should be possible to take this into account when creating codepoints. But it's much more complex than the current algorithm. That approach would not need to store the codepoints. But every deletion have to be taken into account. Is it possible to find out when an icon was deleted?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants