Re: breeze-icons and the symlink business

[Gleb Popov]

On Thu, Feb 2, 2017 at 2:49 AM, Harald Sitter  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).
>
>
> Kindly fix this to empower the artists to know what's going on again :(
> HS
>

This PR might be relevant: https://git.reviewboard.kde.org/r/128112/
I intended to use it on Windows only, but it well may be used on *nix too.

Re: breeze-icons and the symlink business

[Harald Sitter]

On Thu, Feb 2, 2017 at 12:24 PM, Jaroslaw Staniek  wrote:
>
>
> On 2 February 2017 at 00:49, Harald Sitter  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

Re: breeze-icons and the symlink business

[Jaroslaw Staniek]

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

breeze-icons and the symlink business

[Harald Sitter]

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).


Kindly fix this to empower the artists to know what's going on again :(
HS