On 2 February 2017 at 00:49, Harald Sitter <sit...@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