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.

Sign up for GitHub

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

Jump to bottom

build: batch svg files for inkscape #54

Merged
merged 1 commit into from
Nov 27, 2024
Merged

build: batch svg files for inkscape #54

merged 1 commit into from
Nov 27, 2024

Conversation

vdbe
Copy link
Contributor

@vdbe vdbe commented Nov 25, 2024

Batch inkscape for multiple svg files.

vdbe
vdbe commented Nov 25, 2024
View reviewed changes
Copy link
Contributor Author

@vdbe vdbe left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I don't know what the max length of a variable is but maybe it's a good idea to use a file instead,
doesn't seem to be a problem at this moment.

scripts/build-xcursor-plasmasvg
fi
done
if [ "$genPixmaps" != "file-open:${RAWSVG};" ]; then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check is lost don't think it's needed since we no longer call inkscape for every file,
but can be added by setting a flag inside the if above.

Copy link
Contributor

Choose a reason for hiding this comment

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

works fine without

@Covkie
Copy link
Contributor

Covkie commented Nov 26, 2024 •
edited
Loading

what's the advantage of this? If its speed can you provide a benchmark comparison?

edit: forgot that actions exist and are effectively a build benchmark. wow 18 -> 4 mins on full build time, yipppee!

sgoudham
sgoudham approved these changes Nov 27, 2024
View reviewed changes
Copy link
Contributor

@sgoudham sgoudham left a comment

Choose a reason for hiding this comment

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

Tested them out locally and it looks good 👍

They were batched before but I missed this when we ported over the KDE Breeze scripts, nice catch and thanks!

@sgoudham sgoudham merged commit cec18c8 into catppuccin:main Nov 27, 2024
1 check passed
@github-actions github-actions bot mentioned this pull request Nov 27, 2024
Merged
@Covkie Covkie mentioned this pull request Nov 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Covkie Covkie Covkie approved these changes

@sgoudham sgoudham sgoudham approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vdbe @Covkie @sgoudham