On Tue, Aug 22, 2017 at 10:18 PM, Luigi Toscano <luigi.tosc...@tiscali.it> wrote: > Harald Sitter wrote: >> Ahoy ahoy >> >> I've just stumbled upon a rather puzzling situation with kdoctools. It >> has code branching to turn its assets relocatable [1] (i.e. resolve >> paths relative rather than hardcode their location). Now the weird bit >> about this is that it is only used on windows. > > Hi, > > it's not so puzzling: the main code assume a fixed position for the docbook > resources (which has been for long the case, if we consider them system > libraries). The windows code has been added (long time ago, and thanks to the > windows people for that!) as special case because the path required when > building and the path required when installing are a bit different. > > See for reference when it was introduced: > https://commits.kde.org/kdelibs/38b5e7f937b5d2c291c5b20a0c8648632084dde5 > > And this revision when I tried to simplify it: > https://git.reviewboard.kde.org/r/120324/ > https://bugs.kde.org/show_bug.cgi?id=293610
Yeah, I suspected as much. >> The reason this puzzles me is that the relocatable code for Windows >> would work just fine for Linux and OSX, from what I can tell there is >> no real downside to it besides the additional code, which we need >> anyway. On the other hand, the conditional treatment of Windows gives >> the Windows code branch substantially less implicit run exposure (i.e. >> most devs/testers aren't on Windows, so fewer people build the >> relevant if-branch). >> >> With that in mind: how about we drop the harcoding code path and make >> the Windows code path the default and have kdoctools assets always be >> relocatable? > > No problem with relocatable code, in general, but my personal problem with > that code is that I have to rethink every time what it's doing and think twice > when I had to change it (as I did now with the review above), because of the > way it works. It may be a limitation of mine, but is there some way to make > what it's trying to do in a more simple way? Mh, yes. Relativity calculation is always a strain on the mind for sure. The way I see it this is fairly isolated in the cmake logic though and ideally only needs figuring out once, so it shouldn't need changes that often? I mean, from a usage point of view when working on a dtd or xsl you use the same cmake-level variable regardless of the path being absolute or relative anyway. On the cmake-level the principal difference between absolute and relative is that the variable is converted into a relative one using a `file(RELATIVE_PATH ...)` call. Do you have a specific example of where you had trouble wrapping your head around it? FWIW, I think the cmake code at hand would actually be easier on the eyes without the if branch and some simplification. If there are no objections to turning the build relocatable by default I'll go ahead and prepare a diff for review. Then we can talk about tweaking that should it be necessary. HS