| wojnilowicz |
| KMyMoney |
This eliminates the need to install kmymoney and to set some enviroment
variables to find the plugins. They are simply detected from the build
folder.
An additional library search path is required for this because our
plugin folder is named "kmymoney" which collides with the name of our
executable if everything is collected in single folder.
Started KMyMoney from build directory and checked loaded
plugin versions in the settings dialog.
| No Linters Available |
| No Unit Test Coverage |
This eliminates the need to install kmymoney and to set some enviroment
variables to find the plugins. They are simply detected from the build
folder.
Why do you want to eliminate installing? On my machine it takes like less than second and my machine isn't at any point fast.
Why is setting environment variable so upsetting? I've set it once and it isn't impacting my speed or my time.
An additional library search path is required for this because our
plugin folder is named "kmymoney" which collides with the name of our
executable if everything is collected in single folder.
I don't see point in introducing additional library search path for the sake of just being.
Anyhow, I've tested this patch and I see
kf5.kxmlgui: cannot find .rc file "checkprinting.rc" for component "checkprinting"
in logging console. Besides QT_PLUGIN_PATH I also set XDG_DATA_DIRS as pointed out here
https://mail.kde.org/pipermail/kmymoney-devel/2016-June/016920.html
I believe, that because of that I cannot configure my checkprinting plugin.
Your patch somehow infested my builddir, so I had to remove it, so that everything can work again without your patch.
| CMakeLists.txt | ||
|---|---|---|
| 268 ↗ | (On Diff #24880) | Word "even" is unnecessary here. If I see correctly, it isn't meant to change behavior if kmymoney is started from install prefix. |
| kmymoney/main.cpp | ||
| 82 ↗ | (On Diff #24880) | I don't like the idea of adding this path here. It is illogical and completely unneeded in released code. |
| kmymoney/plugins/checkprinting/CMakeLists.txt | ||
| 49–50 | That looks nice and as I see it should put plugin automatically in correct place. | |
Normally, I think the idea to be able to test the program from within the build directory is good. However, this has (I think) always been a problem for KDE, which uses environment variables to find many files necessary to run a program. This is why it takes such effort to be able to have both a KDE4 and KF5 version of an application installed at the same time, to be able to use one and test the other. However, thinking about how I do that, I wonder if the same effect couldn't be had here by using a launch script which sets the necessary variables (pointing to the various areas under the build directory) and then launching the app.
I'm supprised to read this. In D9106 you put a lot of effort to optimize build time in some cases. This patch fits into this effort. However, this is not the reason I made this patch: I did this to simplify the development process, especially to beginners.
An additional library search path is required for this because our
plugin folder is named "kmymoney" which collides with the name of our
executable if everything is collected in single folder.I don't see point in introducing additional library search path for the sake of just being.
Why don't you like the try to add to the library search path? From what I read here this seems to be your main worry about this path.
Anyhow, I've tested this patch and I see
kf5.kxmlgui: cannot find .rc file "checkprinting.rc" for component "checkprinting"
in logging console. Besides QT_PLUGIN_PATH I also set XDG_DATA_DIRS as pointed out here
https://mail.kde.org/pipermail/kmymoney-devel/2016-June/016920.html
I believe, that because of that I cannot configure my checkprinting plugin.
The config files stay at their default location. This patch does not change anything in this regard. The issue of starting KMyMoney to work on plugins comes up regularly, I wrote similar descriptions, too (and more than once).
Your patch somehow infested my builddir, so I had to remove it, so that everything can work again without your patch.
"infested" – do you expect me to reply to this?
Thank you for insightful comment. Surly, I think your suggestion is the only alternative. Also the additional ability to set other paths correctly is valuable but not always required. In any case the script-only solution still requires an make install (or alike) what cannot easily be done by such a script. Additionally this requires the knowledge that the dev has to run the script instead of the executable. My patch is a plug-and-play solution, it allows to start developing quickly (and not looking in mailing lists for a long time). I solved some issues but not all. The best solution is probably using both or even combining it.
| CMakeLists.txt | ||
|---|---|---|
| 268 ↗ | (On Diff #24880) | This does not change kmymoney's behavior at all. It is just a place where CMake will put the libraries inside the build folder. If we use ECM >= 5.38 this will put all binary files into a bin folder anyway (actually that is a pretty good trick). |
| kmymoney/main.cpp | ||
| 82 ↗ | (On Diff #24880) | I did not change anything about installing plugins. QCoreApplication::applicationDirPath() is a directory Qt will look for plugins. However, this cannot be used as stated above. In deed this code is only used during debugging. Btw: If the given folder does not exist, this function does nothing. There are no other paths Qt will look into except system wide paths. Which other issues are you thinking about? I'm also not very happy with this solution but the only other way is the ones suggested by Jack. The reason I used this anyway is, that it is not hurting especially if the given folder does not exist. If a user wants to use undocumented features we discourage from using, he cannot be mad if something breaks. |
| kmymoney/plugins/checkprinting/CMakeLists.txt | ||
| 49–50 | It does not place the plugins into the right place if find_package(ECM 5.38 …) (or any later version) is used. As mentioned previously, it will write to ${CMAKE_LIBRARY_OUTPUT_DIRECTORY}/kmymoney/… but ${CMAKE_LIBRARY_OUTPUT_DIRECTORY}/kmymoney is also our executable. So the manual setting of the output directory will always be required (once we want ECM >= 5.38). The main benefit of using this function is that changes to the json file will trigger the right files to be recompiled. | |
In case of D9106, and alike, benefits to build time are obvious but here I'm not so sure.
Of course it would be nice to get rid of make install and setting environment variables but if this patch is meant do this, then what about XDG_DATA_DIRS?
An additional library search path is required for this because our
plugin folder is named "kmymoney" which collides with the name of our
executable if everything is collected in single folder.I don't see point in introducing additional library search path for the sake of just being.
Why don't you like the try to add to the library search path? From what I read here this seems to be your main worry about this path.
Cause it's very build case specific. Do only I feel, it looks bad here?
Anyhow, I've tested this patch and I see
kf5.kxmlgui: cannot find .rc file "checkprinting.rc" for component "checkprinting"
in logging console. Besides QT_PLUGIN_PATH I also set XDG_DATA_DIRS as pointed out here
https://mail.kde.org/pipermail/kmymoney-devel/2016-June/016920.html
I believe, that because of that I cannot configure my checkprinting plugin.The config files stay at their default location. This patch does not change anything in this regard. The issue of starting KMyMoney to work on plugins comes up regularly, I wrote similar descriptions, too (and more than once).
XDG_DATA_DIRS is not about config files (other variable is for that). This variable contains e.g. kcm .desktop files which are for configuring plugins and it doesn't run from build directory for me.
Additional icons from that dir doesn't get loaded too. Is that expected with your patch?
Your patch somehow infested my builddir, so I had to remove it, so that everything can work again without your patch.
"infested" – do you expect me to reply to this?
Are you kidding? :P What for? I meant to say it doesn't work good here and broke my build dir unexplained.
Thank you for insightful comment. Surly, I think your suggestion is the only alternative. Also the additional ability to set other paths correctly is valuable but not always required. In any case the script-only solution still requires an make install (or alike) what cannot easily be done by such a script. Additionally this requires the knowledge that the dev has to run the script instead of the executable.
The other alternative is to use CMAKE_INSTALL_PREFIX. It's very standard and in my configuration I have to set only two environmental variables for it to run fully i.e. QT_PLUGIN_PATH and XDG_DATA_DIRS.
My patch is a plug-and-play solution, it allows to start developing quickly (and not looking in mailing lists for a long time). I solved some issues but not all. The best solution is probably using both or even combining it.
It doesn't load KMM fully (icons, scripts, kcms,), doesn't it?
| kmymoney/main.cpp | ||
|---|---|---|
| 82 ↗ | (On Diff #24880) |
Why cannot it be used like this? With your patch checkprinting.so already does that. According to QCoreApplication::libraryPaths()
So if we put all plugins in application executable, like with checkprinting.so, then we should be all set. Am I not seeing the big picture? |
| kmymoney/plugins/checkprinting/CMakeLists.txt | ||
| 50 | You put kcm_checkprinting in KMM_PLUGIN_OUTPUT_DIRECTORY but forgot to put | |
From my point of view, the only issue left is the line QCoreApplication::addLibraryPath(QCoreApplication::applicationDirPath() + QDir::separator() + "kmymoney-plugins");. Since this seems to be a highly emotional question so I do not think further discussions are of use here.
I am for pushing this patch, Lukaz is not. So we have a tie and I will be happy to hear the other developers opinions. If more devs are okay to send, I will push it, otherwise I will simply abandon it.
| kmymoney/main.cpp | ||
|---|---|---|
| 82 ↗ | (On Diff #24880) |
Try it with find_package(ECM 5.38 …) and without my set_target_properties(… PROPERTIES LIBRARY_OUTPUT_DIRECTORY "${KMM_PLUGIN_OUTPUT_DIRECTORY}") in at least one plugin. |
| kmymoney/main.cpp | ||
|---|---|---|
| 82 ↗ | (On Diff #24880) | Did you read my last comment? |
| kmymoney/main.cpp | ||
|---|---|---|
| 82 ↗ | (On Diff #24880) | To follow up: Why can't all plugins be made like this one you've made? |