Looking pretty good so far. The only icons that have 24px versions but no 22px versions are the following from the actions category:

24/align-horizontal-node.svg
24/align-vertical-node.svg
24/audio-volume-high.svg        This is wrong anyway
24/audio-volume-low.svg        This is wrong anyway
24/audio-volume-medium.svg        This is wrong anyway
24/audio-volume-muted.svg        This is wrong anyway
24/distribute-horizontal-node.svg
24/distribute-vertical-node.svg
24/font.svg
24/gnumeric-autofilter-delete.svg        Does anyone still use Gnumeric?
24/gnumeric-autosum.svg
24/gnumeric-bucket.svg
24/gnumeric-cells-merge.svg
24/gnumeric-column-size.svg
24/gnumeric-component-insert-shaped.svg
24/gnumeric-data-slicer.svg
24/gnumeric-font.svg
24/gnumeric-format-border-all.svg
24/gnumeric-format-accounting.svg
24/gnumeric-format-border-double-bottom.svg
24/gnumeric-format-border-none.svg
24/gnumeric-format-border-outside.svg
24/gnumeric-format-border-thick-bottom.svg
24/gnumeric-format-border-thick-outside.svg
24/gnumeric-format-border-top-n-bottom.svg
24/gnumeric-format-border-top-n-thick-bottom.svg
24/gnumeric-format-border-top-n-double-bottom.svg
24/gnumeric-format-percentage.svg
24/gnumeric-format-precision-decrease.svg
24/gnumeric-format-precision-increase.svg
24/gnumeric-format-thousand-separator.svg
24/gnumeric-formulaguru.svg
24/gnumeric-link-external.svg
24/gnumeric-link-internal.svg
24/gnumeric-link-url.svg
24/gnumeric-object-arrow.svg
24/gnumeric-object-button.svg
24/gnumeric-object-checkbox.svg
24/gnumeric-object-combo.svg
24/gnumeric-object-ellipse.svg
24/gnumeric-object-label.svg
24/gnumeric-object-line.svg
24/gnumeric-object-list.svg
24/gnumeric-object-rectangle.svg
24/gnumeric-object-scrollbar.svg
24/gnumeric-object-spinbutton.svg
24/gnumeric-pagesetup-hf-cell.svg
24/gnumeric-protection-no.svg
24/gnumeric-pagesetup-hf-time.svg
24/gnumeric-protection-yes.svg
24/gnumeric-row-size.svg
24/gtk-tab-duplicate.svg
24/snap-nodes-cusp.svg
24/snap-nodes-midpoint.svg
24/snap-nodes-path.svg
24/snap-nodes-smooth.svg
24/transform-scale-horizontal.svg
24/transform-scale-vertical.svg
24/y-zoom-in.svg
24/zoom-in-x.svg
24/zoom-out-x.svg
24/zoom-out-y.svg
24/gtk-tab-new.svg
24/media-mout.svg        This is wrong anyway

Out of the ones listed above, these are symlinks:

24/font.svg
24/gnumeric-format-border-all.svg
24/gnumeric-format-border-none.svg
24/gnumeric-format-border-outside.svg
24/gnumeric-object-label.svg
24/gnumeric-object-line.svg
24/gnumeric-pagesetup-hf-cell.svg
24/gnumeric-pagesetup-hf-time.svg
24/gtk-tab-duplicate.svg
24/gtk-tab-new.svg
24/media-mout.svg
24/transform-scale-horizontal.svg
24/transform-scale-vertical.svg

draw-highlight got a bit mangled:

I think it's because both the viewbox and the height/width were set, which made the scaling at 22px equal to 0.26459. This looks fine when we're not trying to make 24px icons, but it obviously messes with the transformation math.

All of the other icons that have height/width and viewbox set have the same problem when converted to 24px:

draw-highlight.svg
edit-line-width.svg
flashlight-on.svg
flashlight-off.svg
minuet-chords.svg
minuet-intervals.svg
minuet-scales.svg
minuet-rhythms.svg
trim-margins.svg
view-sort-ascending.svg
view-sort-descending.svg
view-sort.svg
trim-to-selection.svg

It won't be enough to just remove height/width whenever viewbox is present. We'll either have to figure out the math or we'll have to edit by hand and mandate 1.0 document scale in Inkscape.

If we choose to edit by hand, that will be better off being done in a separate patch after this one.

The following 24px icon symlinks are linked to 22px icons. Maybe it's material for another patch, but I figured I'd bring it up.

configure-shortcuts.svg -> ../../devices/22/input-keyboard.svg
dblatex.svg -> ../../places/22/network-server-database.svg
headphones.svg -> ../../devices/22/audio-headphones.svg
help-contents.svg -> ../../apps/22/system-help.svg
kdenlive-hide-audio.svg -> ../../status/22/audio-volume-muted.svg
kdenlive-show-audio.svg -> ../../status/22/audio-volume-high.svg
kstars_xplanet.svg -> ../../places/22/folder-html.svg
l2h.svg -> ../../places/22/folder-html.svg
new-command-alarm.svg -> ../../places/22/folder-script.svg
player-volume-muted.svg -> ../../status/22/audio-volume-muted.svg
player-volume.svg -> ../../status/22/audio-volume-high.svg
viewhtml.svg -> ../../places/22/folder-html.svg

I've fixed the 22px files that had height/width and viewbox set.

I've added the icons that were only in actions/24 to actions/22.

Seems FreeBSD has an issue with the generation code. Please check https://build.kde.org/view/Failing/job/Frameworks/job/breeze-icons/job/kf5-qt5%20FreeBSDQt5.13/135/console

Please note that the FreeBSD breakage has now started to cause Dependency Builds on the CI system to fail.

looks like it's not posix sh, but bash code

{} expansions are bashism.
Never ever test files meant for sh with bash and be mindful that many distributions will symlink /bin/bash to /bin/sh. There is a huge amount of not so subtle syntax differences between POSIX sh and bash that an sh-only implementation will not know what to do with. https://linux.die.net/man/1/checkbashisms can also be used, but is less reliable than simply running the script through a sh-only implementation.
https://mywiki.wooledge.org/Bashism and https://wiki.ubuntu.com/DashAsBinSh have a fairly comprehensive list of the bigger pitfalls.

Oh, oh boy, there's many more things wrong with this than just the bashism :|

@ndavis @ngraham why don't you generate the 24x version inside the source?

COMMAND ${BASH_EXE} ${CMAKE_SOURCE_DIR}/generate-24px-versions.sh ${BREEZE_INSTALL_DIR} this is bypassing make DESTDIR control, which will make this entire thing unpackable. It'll also outright not work if the target dir is not writable by the user running make (e.g. as is the case during packaging or when a user builds for /usr/local).

There's also a var interpolation bug here: OUTPUT_FILE="$OUTPUT_DIR/${INPUT_FILE/22/24}" (mind the INPUT_FILE expansion closing way after the var name ended)

I'm installing freebsd now to find my way to a fix ^^

Bhushan already has D27614 for the installation problem.

Should we just revert for now?

Also we need to have this on Windows, too.
Breeze-icons is part of Frameworks and these were exported/installed files. A user could easily have a reference to one of the files somewhere.

@davidre we provide a theme, if someone explicitly looks for a specific file within that theme that'd be squarely outside the supported realm of a theme. it'd prevent improving the theme which may mean changing scaling rules and removing the then useless sizes.

I disagree but it's not my call

This is now working again. The symlink tests fail though. That still needs fixing by someone who isn't me.

Here's my feedback:

For bash or sh scripts:

• make sure a sh script is actually POSIX compliant by running it through a POSIX sh implementation
• if the scripts outcome is relevant you must set -e to force all errors to also terminate the script and in turn force you to either deal with them or explicitly ignore them
• if your script is doing a lot of stuff you may want to revise how the script works. specifically for this diff sed needs to iter all the content of all the SVGs this can benefit greatly from running across multiple cores, but with the way it is written it cannot. a better solution would be to move the for loop into cmake and then construct targets for all files where each target calls generate.sh inputfile and the shell script only has the logic from inside the loop. this effectively gives you free "threading" in that cmake/make/ninja know how to run build targets in parallel so by making each file its own target you can now utilize all cores of a system without having to manage threading manually.

On the cmake side:

• do not ever mutate installation paths (well, exceptions apply ofc, but 99.9% of the time). they may not be writable and they will not ever be what you expect them to be because the paths themselves are mutable in packaging scenarios (as in: cmake gets run with path=/usr BUT that's is not at all where things get installed when make install gets called).
• setting working directory to current source is bad form. it allows you to accidentally pollute or change the source dir, it's undesirable and generally speaking relying on relative paths is a bit meh anyway
• bit nitpicky: but generally when you have any find_* call you'll want some cmake FeatureSummary call to go along with that to describe whatever you need the finded thing for. specifically since this script is actually bash-dependent we need to look for bash, but currently we provide no useful feedback on it being missing and the build will just fail on BASH-EXEC_NOT_FOUND or something when the relevant target would get run. a cheap solution is to use add_feature_info to describe the feature

In general for this diff:

• the introduced script absolutely needs a test. its correct behavior is required to have the x24 variants and right now we have no assurances of that
• as mentioned, I really do not like that this runs at build time. it's not very "green". the outcome does not change based on environment, nor is the target environment relevant. so a dozen or more developers regularly needlessly spend power on generating the ever same mutations of the largely ever same files, and every month all distributions do the same, equally needlessly. this should only be run when artists add/change icons

EDIT: actually the icons get generated twice because the target is always out of date, so when you run make and then make install both times the icons are generated -.-

Thanks so much everyone! @sitter I'll read up on CMake a bit and try to implement your suggestions.

Freebsd side broken again :
00:31:45 ./build/icons-dark/generated/devices/24/network-wired-activated.svg:14: parser error : Opening and ending tag mismatch: g line 8 and svg
00:31:45 </svg>
00:31:45 ^
00:31:45 ./build/icons-dark/generated/devices/24/network-wired-activated.svg:15: parser error : EndTag: '</' not found
00:31:45
00:31:45 ^
00:31:45 gmake[2]: * [CMakeFiles/breeze-validate-svg.dir/build.make:57: CMakeFiles/breeze-validate-svg] Error 1
00:31:45 gmake[1]: * [CMakeFiles/Makefile2:213: CMakeFiles/breeze-validate-svg.dir/all] Error 2
00:31:45 gmake[1]: *** Waiting for unfinished jobs....

There is also the issue that the breeze-generate-24px-versions target is not set as dependency for the breeze-validate-svg target. As a result both targets will be processed in parallel if multiple jobs are possible. And on KDE CI, where the "build/" directory is a subdir to the root source directory, this means that the validation will go and process also all generated SVGs (which actually makes sense and should be ensured to always happen), but does so while they are still generated.
This results in the random validation errors currently seen on KDE CI, when xmllint goes for a SVG which has been only partially generated and e.g. yet missing the final </g> tag.

@ngraham @ndavis This has been broken for quite some days now on KDE CI (cmp. also @usta's comment from some days ago) please take responsibility for your work.

Pushed 004cf12a2fdb09ab8b62e3f8d84e1584fe08ebab as intermediate hotfix to help CI and other parallel builds. The whole logic needs a clean rework though, e.g. to make sure the generated icons are always validated as well (given the sed processing is prone to errors with future icons by a chance).

Sorry for all the trouble. Unfortunately, neither Nate or I have much experience with CMake or the gotchas of cross platform shellscript and Unix tool support. We'll have to be a lot more careful next time and stick to something with fewer gotchas like Python.

Sorry, I was busy at a hackathon this weekend and am only just now catching up on KDE stuff. Thanks for the hotfix.

To echo what Noah said, we'll be more careful in the future. This script probably needs to be rewritten in Python and the loop moved into CMake, as @sitter suggested--or do you think it would be possible to do the whole thing in CMake? It looks like there are facilities to copy files and create symlinks. But to be honest, I find CMake to be pretty frustrating and unfriendly to work with. :(

It's doable in cmake but it'd be awkward and needlessly complex I think, you are much better served putting the actual processing into a script.

ki18n's cmake/ macros have function with very similar tech. In fact, KI18N_INSTALL probably is super close in the functionality. It generates .mos out of the .pos at build time, while you want to generate 24px out of 22px.

In D27557#616831, @sitter wrote:

• if your script is doing a lot of stuff you may want to revise how the script works. specifically for this diff sed needs to iter all the content of all the SVGs this can benefit greatly from running across multiple cores, but with the way it is written it cannot. a better solution would be to move the for loop into cmake and then construct targets for all files where each target calls generate.sh inputfile and the shell script only has the logic from inside the loop. this effectively gives you free "threading" in that cmake/make/ninja know how to run build targets in parallel so by making each file its own target you can now utilize all cores of a system without having to manage threading manually.

1. I’m sure most build systems don’t run on Tianhe-2, so we don’t need one thread per icon.
2. If someone uses make instead of ninja, make will take very much time to fire up the 2000 (11000 if we also auto-generate dark versions) threads.

I think it makes more sense to define one target for every directory, and then 4 (43) parralel-running Python scripts iterate through their own directory. Then we also don’t need a for loop in cmake.

By the way that inefficient shell script needs only 18 seconds on my slowbook. I think that is fast enough and competitively green enough, and a streamlined Python script will probably be even better.