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

Add caching option #1

Closed
ronaldtse opened this issue Jan 26, 2024 · 15 comments · Fixed by #6
Closed

Add caching option #1

ronaldtse opened this issue Jan 26, 2024 · 15 comments · Fixed by #6
Assignees
Labels
enhancement New feature or request
Projects

Comments

@ronaldtse
Copy link
Contributor

This action to setup Fontist takes around 1 minute.

We should add a cache option so that the setup is cached.

@ronaldtse ronaldtse added the enhancement New feature or request label Jan 26, 2024
@ronaldtse ronaldtse added this to TRIAGE in Fontist via automation Jan 26, 2024
@ronaldtse
Copy link
Contributor Author

@CAMOBAP can you please help here? Thanks!

@ronaldtse ronaldtse moved this from TRIAGE to High priority in Fontist Jan 26, 2024
@jcbhmr

This comment was marked as outdated.

@jcbhmr
Copy link
Collaborator

jcbhmr commented Jan 26, 2024

in #6 the caching of the installed actual Ruby files is taken care of. that's where the 1minute install time (to run gem install fontist) came from. that's unsolvable for first-time runs but can be solved for subsequent runs using RUNNER_TOOL_CACHE in #6 this is wrong; see #6 (comment)

there's another level of "dependency caching" that can be done sorta like how the setup-node project will cache node_modules. this is only relevant if the ~/.fontist folder is so huge that redownloading/creating it takes longer than restoring if from cache (which still involves a download; just from actions/cache not from the formulas repo or whatever)

bun for example doesn't use a cache because downloading fresh is faster than fetching from cache oven-sh/setup-bun#14 (comment)

other relevant discussion similar usecase typst-community/setup-typst#15 (comment)

@CAMOBAP
Copy link

CAMOBAP commented Jan 26, 2024

this is only relevant if the ~/.fontist folder is so huge that redownloading/creating it takes longer than restoring if from cache (which still involves a download; just from actions/cache not from the formulas repo or whatever)

It looks relevant because fonts may be stored on third-party over the world and it's more about reliability them for speed

@jcbhmr
Copy link
Collaborator

jcbhmr commented Jan 26, 2024

another question to bring up is: what would be the cache key? there's a manifest.yml mentioned in https://www.fontist.org/docs/ that could be used like hashFiles("manifest.yml")

@CAMOBAP
Copy link

CAMOBAP commented Jan 26, 2024

another question to bring up is: what would be the cache key? there's a manifest.yml mentioned in https://www.fontist.org/docs/ that could be used like hashFiles("manifest.yml")

IMHO caching without prior #4 implementation will have not much sense (otherwise users can use cache step on their own with better flexibility and control)

So we can use font string to calculate hash

@ronaldtse @jcbhmr make sense?

@jcbhmr
Copy link
Collaborator

jcbhmr commented Jan 26, 2024

IMHO caching without prior #4 implementation will have not much sense (otherwise users can use cache step on their own with better flexibility and control)

that's not typically how things are used; see https://github.com/actions/setup-node and https://github.com/actions/setup-python for good examples.

the idea is that you:

  1. install $TOOL which restores any previous hashFiles(lockfile) to either ./deps or ~/deps or something
  2. you run package-manager ci or packagemanager install installs things if they dont exist to ./deps or ~/deps
  3. post-run action step from the setup-$TOOL action runs which takes the ./deps or ~/deps and saves them in an os-specific cache based on the hashFiles(lockfile) from before

@CAMOBAP
Copy link

CAMOBAP commented Jan 26, 2024

Good catch because caching happens on post-run action we still can do it with some hardcoded or user-defined key

The question is: are potential users interested in calling fontist explicitly at all?

As far as I understand the purpose of the action the goal is to install fonts. And users probably don't care how exactly installation will happen (correct me if I'm wrong)

At this moment, it looks like we should provide all required input into the setup action to install all required fonts in one place i.e. fonts, manifest maybe families

@jcbhmr
Copy link
Collaborator

jcbhmr commented Jan 26, 2024

As far as I understand the purpose of the action the goal is to install fonts.

To be more specific: the goal is to setup Fontist; specifically to install the Fontist CLI and (probably) run fontist update to prep the local index.

Typical use probably looks like:

- uses: fontist/setup-fontist@v1
- run: fontist list "Fira Code" > report.txt
- run: node process-report.js report.txt
- run: fontist install "Consolas"
- run: fontist manifest-install manifest.yml
- run: typst paper.typ --font-path ~/.fontist/fonts
- run: cp ~/.fontist/fonts ./fonts && npm run build-web-app-with-fonts

Notice how we just setup the Fontist environment and let the user do the installing and such. This is similar to how setup-node and setup-python leave the whole "how do you want to use this Node.js or Python installation?" up to the user completely.

tl;dr: install and configure the cli but dont do anything over-the-top; let the user use the cli to do things instead of wrapping the cli in YAML config mess

@CAMOBAP
Copy link

CAMOBAP commented Jan 26, 2024

@jcbhmr see your point thanks,

To me, it looks like we should not cache anything else except ~/.fontist

@jcbhmr
Copy link
Collaborator

jcbhmr commented Jan 26, 2024

bringing things back around to caching, here's the three things i think COULD be cached:

  1. the ruby installation
  2. the gem install fontist ruby files that get downloaded
  3. the ~/.fontist/something folders with indexed metadata/formulas

the ruby installation is taken care of https://github.com/ruby/setup-ruby

btw i was wrong about #6 solving the 2 "cache the gem install fontist" thing: #6 (comment) tldr it doesnt

@CAMOBAP
Copy link

CAMOBAP commented Jan 26, 2024

  1. I can't believe that the whole ruby installation is actually 'cached' by setup-ruby, can you confirm it? Most popular versions of ruby are already preinstalled on the runner, what exactly you propose to cache?
  2. This can be done by generation Gemfile during the action run and cache it + bundler-cache: true
  3. simple actions/cache step, composite actions allow to include it

P.S. just found that you already worked on it #6 )

@jcbhmr
Copy link
Collaborator

jcbhmr commented Jan 26, 2024

bundle install
Could not determine gemfile path, skipping "bundle install" and caching> I can't believe that the whole ruby installation is actually 'cached' by setup-ruby, can you confirm it? Most popular versions of ruby are already preinstalled on the runner, what exactly you propose to cache?

Yep! It installs whatever ruby-version: ruby-3.1.0 you want by downloading special binaries specifically tailored for GitHub actions from https://github.com/ruby/ruby-builder and installs it to $RUNNER_TOOL_CACHE which is cached between runs of the same runner

Why not use the builtin system Ruby installation? Good point I didn't realize GitHub Actions runners came with Ruby preinstalled. I guess for Docker-based workflows like https://docs.github.com/en/actions/using-jobs/running-jobs-in-a-container that don't have Ruby installed?

You're right we could drop the uses: ruby/setup-ruby from action.yml and be fine. 🤷‍♀️

This can be done by generation Gemfile during the action run and cache it + bundler-cache: true

I was under the impression that this ran bundle install and then cached the /var/rubygemlocation/* so I don't think it would work with gem install fontist globally? Maybe im wrong.

For reference I just tried a blank repo with this workflow file:

name: My workflow
on: [push, pull_request]
jobs:
  test:
    runs-on: ubuntu-latest
    steps:
    - uses: actions/checkout@v4
    - uses: ruby/setup-ruby@v1
      with:
        ruby-version: '3.3' # Not needed with a .ruby-version file
        bundler-cache: true # runs 'bundle install' and caches installed gems automatically
    - run: gem install fontist

and it DID NOT cache the gems: 😢

> bundle install
Could not determine gemfile path, skipping "bundle install" and caching

or did you mean install fontist using a echo "whatever"> /tmp/Gemfile and then install from that? Because that might work!

...only problem is that setup-ruby caches the ENTIRE /var/rubgeminstallfolder so this would include any user-installed dependencies as well. Unsure if you want that or how well that would work in an existing Ruby project. I'm not a Ruby expert. 🤷‍♀️

simple actions/cache step, composite actions allow to include it

👍

@jcbhmr
Copy link
Collaborator

jcbhmr commented Jan 26, 2024

OHHH i remember "why use setup-ruby" and isolate the ruby installation? because what if a user does this:

- uses: ruby/setup-ruby@v1
  with:
    ruby-version: "2.x"
- uses: fontist/setup-fontist@v1

what happens now? should we use system ruby or install our own copy using another setup-ruby invocation?

you could just say "psssh unsupported; get a good ruby version or else it just doesn't work 🤷‍♀️" lol which is probably the best thing to do honestly

@jcbhmr jcbhmr mentioned this issue Jan 27, 2024
@jcbhmr
Copy link
Collaborator

jcbhmr commented Jan 27, 2024

i created #8 and #7 for my own organizational ocd. i wanted to organize and write down my thoughts a little more coherently than I have been doing in this thread convo lol. hope that's ok with u 😄

Fontist automation moved this from High priority to Done Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Fontist
  
Done
Development

Successfully merging a pull request may close this issue.

3 participants