Re: relocatable kdoctools

[Ralf Habacker]

Am 29.08.2017 um 14:16 schrieb Harald Sitter:
> On Thu, Aug 24, 2017 at 3:20 PM, Ralf Habacker  
> wrote:
>> 10. The conclusion of 9. is that it might also be a solution on windows
>> to patch only all generated files after 3. and before 4.
> Hm, I may be missing a piece of the puzzle in my mind, but it appears
> to me this should be working with the cmake code paths at hand, no? I
> mean, it appears the paths for that mingw32 build are a bit all over
> the place, but if that wasn't the case it should be fine. Which seems
> where the sed in your point 9 comes in. Your actually generated
> relativity is incorrect considering what you want/need.
>
> i.e. ../../../../../../../../../share (which supposedly goes to
> /usr/share/ on the build system itself)
/usr/i686-w64-mingw32/sys-root/mingw/share
> vs. ../../../.. (which I do not know where it is meant to go but you
> expec the docbook to be there)
>
> So I think the problem is your relativity more than anything. How to
> fix that I can't say as I lack some background knowledge of how
> mingw32 works on opensuse.
mingw32 packages uses an install root of
/usr/i686-w64-mingw32/sys-root/mingw/
>
> From a windows POV we'd have this line up though:
>
> /c/kdoctools/dist/ <-- destination for distributable dir
> /c/kdoctools/dist/data/xml/docbook <-- docbook assets for distribution
> (loaded by kdoctools dtd/xsl files)
see
https://build.opensuse.org/build/home:rhabacker:branches:windows:mingw:win32:KF536/openSUSE_Leap_42.2/x86_64/mingw32-kdoctools/_log
at  timestamp '118s'

Ralf

Re: relocatable kdoctools

[Ralf Habacker]

Am 29.08.2017 um 15:03 schrieb Harald Sitter:
> On Sun, Aug 27, 2017 at 8:37 PM, Luigi Toscano  
> wrote:
>> Even without the if branch it would be still complicated.
> Ah! I think I get the complexity now. At build time we need to run our
> tools already and so the dtd/xsl need to be able to resolve the paths,
> hence the initial configure_file with absolute path 
that is what I mentioned with

>3. cmake uses the generated files from 1. to generate kdoctools
> documentation

> which then get replaced for win32 with newly generated relative dtd/xsl files 
> at
> install time.
see topic 4.

> 4. kdoctools installs generated file for usage by other packages
with the addition of "installs second set" 

>  Indeed that is tricky to fix. Well, kind of.
>
> What we can do is get rid of the `install(CODE ...)` stuff. We know
> the actual paths at configure time just fine, we only can't use them
> for the files generated at build time. I am not sure that is much
> better, but it would certainly remove the excessive escaping need.
>
> The only way to make this really transparent would be to copy the
> docbook assets into build dir and basically construct the directory
> lineup as it will look after installation, so during build-time assets
> behave exactly like after installation. That's probably more
> complicated useless code than we have now though.
>
> HS

Re: relocatable kdoctools

[Harald Sitter]

On Sun, Aug 27, 2017 at 8:37 PM, Luigi Toscano  wrote:
> Even without the if branch it would be still complicated.

Ah! I think I get the complexity now. At build time we need to run our
tools already and so the dtd/xsl need to be able to resolve the paths,
hence the initial configure_file with absolute path which then get
replaced for win32 with newly generated relative dtd/xsl files at
install time. Indeed that is tricky to fix. Well, kind of.

What we can do is get rid of the `install(CODE ...)` stuff. We know
the actual paths at configure time just fine, we only can't use them
for the files generated at build time. I am not sure that is much
better, but it would certainly remove the excessive escaping need.

The only way to make this really transparent would be to copy the
docbook assets into build dir and basically construct the directory
lineup as it will look after installation, so during build-time assets
behave exactly like after installation. That's probably more
complicated useless code than we have now though.

HS

Re: relocatable kdoctools

[Harald Sitter]

On Thu, Aug 24, 2017 at 3:20 PM, Ralf Habacker  wrote:
> 10. The conclusion of 9. is that it might also be a solution on windows
> to patch only all generated files after 3. and before 4.

Hm, I may be missing a piece of the puzzle in my mind, but it appears
to me this should be working with the cmake code paths at hand, no? I
mean, it appears the paths for that mingw32 build are a bit all over
the place, but if that wasn't the case it should be fine. Which seems
where the sed in your point 9 comes in. Your actually generated
relativity is incorrect considering what you want/need.

i.e. ../../../../../../../../../share (which supposedly goes to
/usr/share/ on the build system itself)
vs. ../../../.. (which I do not know where it is meant to go but you
expec the docbook to be there)

So I think the problem is your relativity more than anything. How to
fix that I can't say as I lack some background knowledge of how
mingw32 works on opensuse.

>From a windows POV we'd have this line up though:

/c/kdoctools/dist/ <-- destination for distributable dir
/c/kdoctools/dist/data/xml/docbook <-- docbook assets for distribution
(loaded by kdoctools dtd/xsl files)
/c/kdoctools/build <-- cmake build dir
/c/kdoctools/source <-- cmake build dir

The original docbook assets in there come from building docbook which
we'll need to build kdoctools.

We'd invoke cmake in `/c/kdoctools/build` with something like
`cmake /c/kdoctools/source -DCMAKE_INSTALL_PREFIX=/c/kdoctools/dist`

Which will find the assets in `/c/kdoctools/dist/data/docbook`. It
then proceeds to calculate the relativity from where we want to
install our assets. We'll install (I think) to
`/c/kdoctools/dist/data/kdoctools/customization/dtd/foo.dtd`, which
will result in a realtive load path for the docbooks assets in
`../../../docbook` that is the path that gets embedded into our
foo.dtd.

After installation the line up will look something like this (mostly
guessing since I have no working win32 build system right now):
/c/kdoctools/dist/meinproc5.exe
/c/kdoctools/dist/data/kdoctools
/c/kdoctools/dist/data/xml/docbook
/c/kdoctools/dist/...

This dist directory is relocatable. The foo.dtd in data/kdoctools will
always find its docbook assets in the same relative path to itself.

HS

Re: relocatable kdoctools

[Luigi Toscano]

Harald Sitter wrote:
> On Tue, Aug 22, 2017 at 10:18 PM, Luigi Toscano
>  wrote:
>> Harald Sitter wrote:

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

Even without the if branch it would be still complicated. But I have no
objections to discuss and approve a patch which unifies the code and also
avoids that opensuse-specific patch.

Ciao
-- 
Luigi

Re: relocatable kdoctools

[Ralf Habacker]

Am 23.08.2017 um 14:15 schrieb Harald Sitter:
> On Tue, Aug 22, 2017 at 10:47 PM, Ralf Habacker
>  wrote:
>> Hi,
>>
>> I'm using the following patch to cross build kdoctools for windows
>>
>> https://build.opensuse.org/package/view_file/home:rhabacker:branches:windows:mingw:win32:KF536/mingw32-kdoctools/0001-Generate-xml-files-containing-relative-pathes-to-dtd.patch?expand=1
>>
>> It requires only a little platform specific code - may be it helps.
> Are you sure the xsl mangling is necessary? This should work out of
> the box with the code we have.
>
> What's the rationale behind the docbookl10nhelper change?
Let me give some overview, so that we have the same point of  view:

1. kdoctools generates the following files

- /src/customization/kde-include-common.xsl
- /src/customization/kde-include-man.xsl
- /src/customization/xsl/all-l10n.xml

2. The generated files in 1. have absolute build time pathes inside

3. cmake uses the generated files from 1. to generate kdoctools
documentation

4. kdoctools installs generated file for usage by other packages

5. on installing the package, the generated files need to have relative
pathes on windows patforms.

6. For the generated *.xsl files this should be performed by
src/CMakeLists.txt  (see "customize includes... ") by generating a
second set of files only for installation

7. all-l10n.xml is not covered by 6. because it is generated by
docbookl10nhelper

8. The docbookl10nhelper change in the above mentioned patch makes it
possible to generate two variants of all-l10n.xml, one with absolute for
internal usage and one with relative pathes (add additional relpath
parameter) for installation.

9. The above mentioned patch was my initial try to fix cross compiling
at
https://build.opensuse.org/package/show/home:rhabacker:branches:windows:mingw:win32:KF536/mingw32-kdoctools
but is incorrect in patching cmake files because i did not recognize 3.
at that time. I got a working version by not using the patch instead the
installed files has been patched, (see
https://build.opensuse.org/package/view_file/home:rhabacker:branches:windows:mingw:win32:KF536/mingw32-kdoctools/mingw32-kdoctools.spec?expand=1
line 96ff)

10. The conclusion of 9. is that it might also be a solution on windows
to patch only all generated files after 3. and before 4.

Ralf

Re: relocatable kdoctools

[Harald Sitter]

On Tue, Aug 22, 2017 at 10:47 PM, Ralf Habacker
 wrote:
> Hi,
>
> I'm using the following patch to cross build kdoctools for windows
>
> https://build.opensuse.org/package/view_file/home:rhabacker:branches:windows:mingw:win32:KF536/mingw32-kdoctools/0001-Generate-xml-files-containing-relative-pathes-to-dtd.patch?expand=1
>
> It requires only a little platform specific code - may be it helps.

Are you sure the xsl mangling is necessary? This should work out of
the box with the code we have.

What's the rationale behind the docbookl10nhelper change?

HS

Re: relocatable kdoctools

[Harald Sitter]

On Tue, Aug 22, 2017 at 10:18 PM, Luigi Toscano
 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

Re: relocatable kdoctools

[Ralf Habacker]

Hi,

I'm using the following patch to cross build kdoctools for windows

https://build.opensuse.org/package/view_file/home:rhabacker:branches:windows:mingw:win32:KF536/mingw32-kdoctools/0001-Generate-xml-files-containing-relative-pathes-to-dtd.patch?expand=1

It requires only a little platform specific code - may be it helps.

Ralf


Am 22.08.2017 um 22:18 schrieb Luigi Toscano:
> 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
>
>> 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?
>
> Ciao

Re: relocatable kdoctools

[Luigi Toscano]

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

> 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?

Ciao
-- 
Luigi

Re: relocatable kdoctools

[Harald Sitter]

On Tue, Aug 22, 2017 at 11:32 AM, Ben Cooksley  wrote:
> On Tue, Aug 22, 2017 at 9:25 PM, Harald Sitter  wrote:
>> Ahoy ahoy
>
> Hi Harald,
>
>>
>> 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.
>>
>> 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?
>
> The CI system relocates binaries and other resources on FreeBSD, so
> this should be working on other platforms as well (otherwise anything
> that depends on KDocTools would fail to compile on CI) unless i've
> missed something.

>From what I see this only affects resolution of external assets
actually (e.g. shared schemas from docbook-xml), so this wouldn't
affect CI as /usr/share/xml/ doesn't change location between jobs.

> In general though: relocation is always a good thing. Hardcoded paths
> are a bad thing.

Indeed :)

HS

Re: relocatable kdoctools

[Ben Cooksley]

On Tue, Aug 22, 2017 at 9:25 PM, Harald Sitter  wrote:
> Ahoy ahoy

Hi Harald,

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

The CI system relocates binaries and other resources on FreeBSD, so
this should be working on other platforms as well (otherwise anything
that depends on KDocTools would fail to compile on CI) unless i've
missed something.

In general though: relocation is always a good thing. Hardcoded paths
are a bad thing.

>
> [1] 
> https://phabricator.kde.org/source/kdoctools/browse/master/src/CMakeLists.txt;8c32e153fae80186375d83dbab82bcfc228b1484$9

Cheers,
Ben