breeze-icons and the symlink business

[Harald Sitter]

On Thu, Feb 2, 2017 at 12:24 PM, Jaroslaw Staniek <staniek at kde.org> wrote:
>
>
> On 2 February 2017 at 00:49, Harald Sitter <sitter at kde.org> wrote:
>>
>> hola
>>
>> breeze-icons uses lots of symlinks. Unfortunately, ever so often our
>> icon designers make a mistake and create a bad symlink. To mitigate
>> this I added a bunch of tests making sure everything is nice and
>> dandy.
>>
>> In the mean parts of the build were changed to not tolerate broken
>> symlinks. While I don't really have a problem with that in of itself,
>> the code largely simply ignores the possibility of broken symlinks and
>> fails with the most shitty error messages you could think of. Given
>> our artists are not software engineers they are having a hard time
>> figuring out what is going on. And TBH, I too had to stat files to get
>> to the bottom of the errors. This is a fairly shit situation as on the
>> one hand we want lovely icons and on the other hand the people working
>> on the icons can't understand what needs fixing without having to find
>> a developer they aren't too afraid of to talk to.
>>
>> This really needs fixing.
>>
>> Notably offenders I had a fight with today:
>>
>>
>> # breeze-validate-svg (introduced by Jos)
>>
>> This is a bash script running xmllint.
>>
>> ## Problem 1: Sources
>>
>> The custom target sets `SOURCES ${SVGS}` this has no notable advantage
>> other than making the svgs show up in an IDE, it does however mean
>> that cmake will try to inspect them (as a build tool does when they
>> get told something is a source) and then falls flat on the face when
>> it encounters a broken symlink as it now can't determine the source
>> type resulting in this lovely error
>>
>> ```
>> 21:39:07 CMake Error at CMakeLists.txt:70 (add_custom_target):
>> 21:39:07   Cannot find source file:
>> 21:39:07
>> 21:39:07
>> /home/jenkins/sources/breeze-icons/kf5-qt5/icons-dark/categories/32/applications-other.svg
>> ```
>>
>> ## Problem 2: The code
>>
>> The bash code in of itself runs find with -exec on xmllint. Problem
>> being that if the symlink is broken xmllint will (rightfully) complain
>>
>> ```
>> warning: failed to load external entity
>> "./icons-dark/categories/32/applications-other.svg"
>> ```
>>
>> While that is much better than the earlier problem it's still plenty
>> unobvious what the underlying cause for this is. Supposedly the script
>> should skip bogus symlinks.
>>
>> ## Problem 3: Oh but really
>>
>> This isn't really related to the issue of bad symlink handling:
>>
>> - apparently this didn't get a review. why ever would this not get a
>> review?
>> - this should be a test and only run when testing is enabled
>> - when xmllint is not present it should report this in some form or
>> fashion during the test run so one knows linting is not done
>> - it should record its complaints via ctest so jenkins can display them
>> properly
>> - I fail to appreciate the reason this needs to depend on bash (versus
>> sh, or well, neither)
>>
>> ## Fix Suggestion
>>
>> - don't needlesly set SOURCE
>> - don't pass bad paths to xmllint
>> - deal with stuff in problem 3
>>
>>
>> # RCC generation (introduced by Gleb, enabled by Jaroslaw)
>>
>> This is a bunch of cmake rigging and helper binaries to assemble the
>> icons into an rcc.
>>
>> ## Problem
>>
>> `cmake -E copy_directory` is used to copy the src tree of the themes
>> into the build dir from which the resource file gets assembled. I am
>> guessing copy_directory does not preserver, but resolve symlinks
>> because it greets us with the ever so opaque error:
>>
>> ```
>> Error copying directory from
>> "/home/me/src/git/breeze-icons/icons-dark" to
>> "/home/me/src/git/breeze-icons/build/icon
>> s-dark/res".
>> ```
>>
>> ## Fix Suggestion
>>
>> This is slightly less trivial since the rcc/qrc helpers seem to depend
>> on resolved symlinks, so I am guessing a way to deal with this would
>> be to use cmake's `file(COPY...)` instead of copy_directory and then
>> have another helper run through the directories to flatten out the
>> symlinks (dropping broken symlinks).
>>
>
> Thanks for so detailed research, Harald.
> For the problem #3 I am wondering why the copying should be performed at all
> if a
>
> symlink is invalid. If I understand correctly, how about checking the
> symlinks first and if there are no issues, copying which will go OK?
> That's assuming the symlinks check is not a part of autotests but the actual
> build.

The reason it is an autotest right now is because failure in a test is
more structured and easier to read in jenkins. That said, if you don't
want to skip broken links during the copy we could move the symlink
check to build time at the cost of having it slightly harder to find
the output. It certainly beats the obscure errors we have right now.

Personally, I think