breeze-icons and the symlink business

[Jaroslaw Staniek]

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.

-- 
regards, Jaroslaw Staniek

KDE:
: A world-wide network of software engineers, artists, writers, translators
: and facilitators committed to Free Software development - http://kde.org
Calligra Suite:
: A graphic art and office suite - http://calligra.org
Kexi:
: A visual database apps builder - http://calligra.org/kexi
Qt Certified Specialist:
: http://www.linkedin.com/in/jstaniek
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170202/7372eeb4/attachment-0001.html>