<table><tr><td style="">sitter added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D27557">View Revision</a></tr></table><br /><div><div><p>This is now working again. The symlink tests fail though. That still needs fixing by someone who isn't me.</p>

<p>Here's my feedback:</p>

<p>For bash or sh scripts:</p>

<ul class="remarkup-list">
<li class="remarkup-list-item">make sure a sh script is actually POSIX compliant by running it through a POSIX sh implementation</li>
<li class="remarkup-list-item">if the scripts outcome is relevant you must <tt style="background: #ebebeb; font-size: 13px;">set -e</tt> to force all errors to also terminate the script and in turn force you to either deal with them or explicitly ignore them</li>
<li class="remarkup-list-item">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 <tt style="background: #ebebeb; font-size: 13px;">generate.sh inputfile</tt> 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.</li>
</ul>

<p>On the cmake side:</p>

<ul class="remarkup-list">
<li class="remarkup-list-item">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 <tt style="background: #ebebeb; font-size: 13px;">make install</tt> gets called).</li>
<li class="remarkup-list-item">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</li>
<li class="remarkup-list-item">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 <tt style="background: #ebebeb; font-size: 13px;"> add_feature_info</tt> to describe the feature</li>
</ul>

<p>In general for this diff:</p>

<ul class="remarkup-list">
<li class="remarkup-list-item">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</li>
<li class="remarkup-list-item">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</li>
</ul></div></div><br /><div><strong>REPOSITORY</strong><div><div>R266 Breeze Icons</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D27557">https://phabricator.kde.org/D27557</a></div></div><br /><div><strong>To: </strong>ngraham, VDG, ndavis, Frameworks, sitter<br /><strong>Cc: </strong>davidre, bcooksley, kossebau, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns<br /></div>