Skip to content

Update font URLs and add additional error handling #7418

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

emilykl
Copy link
Contributor

@emilykl emilykl commented May 6, 2025

We have been getting "529 Too Many Requests" errors when downloading fonts in the CI.

It seems these were due to multiple redirects happening for each font URL.

  • I've added the font files directly into the repo to sidestep all of this in the future (I don't love adding 10MB of size to the repo, and open to removing them in the future if it causes issues, but I also don't want to be spending any more time fixing the font downloads every few weeks)

  • I've updated the URLs in the script to use the "final" URL to avoid redirects, and this seems to resolve the issue -- download works successfully now

  • I've also set allow_redirects=False in the request and added logging to print the new URL so the script can be updated, so we can catch this earlier next time

  • I've updated the script to skip downloading the fonts if the file is already present, so while the download script will still run in the CI, it won't actually execute the downloads since the files are in the repo now

  • I've also updated the .circleci/env_image.sh script so that it will fail if any step fails, which was not the case before, which made this issue harder to debug

@gvwilson gvwilson added P1 needed for current cycle fix fixes something broken performance something is slow labels May 8, 2025
@@ -44,7 +44,7 @@ jobs:
install-firefox: false
install-geckodriver: false
install-chrome: true
chrome-version: "127.0.6533.119"
chrome-version: "136.0.7103.92"
Copy link
Contributor

Choose a reason for hiding this comment

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

why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CI was failing with a message about "Chrome failed to start" and this resolved the issue; in any case, it seems good to keep the Chrome version somewhat up to date.

outfile = dir_out + name
print("Getting: ", url)
if os.path.exists(outfile):
print(" => Already exists: ", outfile)
Copy link
Contributor

Choose a reason for hiding this comment

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

rather than overwrite (I don't know how often font files themselves update)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this instead of removing the download step from the CI; in other words, the download script will still run but it will not actually attempt to download the fonts because they are already in the repo.

I could alternatively modify the script to overwrite, and remove the step that runs the script from the CI. I liked this approach though because it's easier to revert if we change our minds about keeping the fonts in the repo.

I don't think we're worried about the font files being updated all that often... if we were I wouldn't want to put them in the repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix fixes something broken P1 needed for current cycle performance something is slow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants