Fix make install by not using a non-existant work dir
AbandonedPublic

Authored by staniek on Jul 15 2016, 10:23 AM.

Details

Reviewers
heikobecker
Group Reviewers
KEXI
Summary

Similar to 9b453a4233a920c918a665a0039b03cec5e8c4bb which fixed this
at build time.

Test Plan

Successfully ran make install to install kexi to my system.

Diff Detail

Repository
R71 Kexi
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
heikobecker retitled this revision from to Fix make install by not using a non-existant work dir.
heikobecker updated this object.
heikobecker edited the test plan for this revision. (Show Details)
heikobecker added a reviewer: KEXI.
Restricted Application added a project: KEXI. · View Herald TranscriptJul 15 2016, 10:23 AM

Thanks Heiko. Could you please provide a full error log from your build? The path should exist, it's created by the fetch_global_breeze_rcc target.

Thanks Heiko. Could you please provide a full error log from your build? The path should exist, it's created by the fetch_global_breeze_rcc target.

Ah, the problem is that my package manager sandboxes the build and does not allow network access during compiling. Nevertheless, here's the full log:
https://gist.githubusercontent.com/anonymous/924cc25357c6a4486935cbd5b222b1d9/raw/806986ba7ba244203069ae2482dd56ffb8c2206d/varlogpaludis1468657829-install-app-office_kexi-scm:0::kde.out

I could probably fetch the extra repo in an earlier phase, but I'm wondering if this wouldn't be better solved by a git submodule? Can't say for sure, but as far as I know, this will be a problem for other distros as well.

I realize while the potential trouble this causes. Please expect external repository and some cmake script that adds the dependency. My motivation for the current solution was to be 100% sure we're getting 1. right icons (the original breeze icons repo can change without keeping compatibility), 2. only icons we may ever need (breeze icons repo contains many extra large files).

So I propose not to apply the patch. Fixed 'make install' does not mean Kexi works at runtime.

Added @bcooksley as we discussed the thing at T3189.

rwooninck added a subscriber: rwooninck.EditedJul 19 2016, 7:56 AM

Please keep in mind that distributions would have similar issues when packaging Kexi. If fetching files during build is a must, then this would mean that we (openSUSE) can not provide any Kexi package to our users. The Open Build Service does not allow network access during build time. This is causing the error that Heiko indicated and I also emailed to you.

I am sure that also distributions like Fedora, etc follow a similar approach and this would mean that with this tactic, users would need to build Kexi themselves.

This comment was removed by staniek.

(added Boud and Dmitry as subscribers since Krita is mentioned)

Hi, I mistakenly typed Krita, but I meant Kexi. The problem does not exist with Krita :)

staniek removed subscribers: dkazakov, rempt.EditedJul 19 2016, 8:10 AM

(added Boud and Dmitry as subscribers since Krita is mentioned)

Hi, I mistakenly typed Krita, but I meant Kexi. The problem does not exist with Krita :)

Ah OK. Please note, even before a pre-release the issue will go so we don't need to even consider someone wouldn't want or be able to package things...

In the meantime, kexi.git has new dependencies: kdb.git, kproperty.git and kreport.git. It would be good to have them packaged (if not already); and all autotests passing.

Sorry for not digging deep enough at first.

I realize while the potential trouble this causes. Please expect external repository and some cmake script that adds the dependency. My motivation for the current solution was to be 100% sure we're getting 1. right icons (the original breeze icons repo can change without keeping compatibility), 2. only icons we may ever need (breeze icons repo contains many extra large files).

Would an option, say DISTRO_BUILD or something similar named, be acceptable? Doing something like that:

if(DISTRO_BUILD)

find_package(breeze-icons)
set_target_properties(... TYPE RUNTIME)

else()

fetch & process breez-icons like it is done now

endif()

Sorry for not digging deep enough at first.

I realize while the potential trouble this causes. Please expect external repository and some cmake script that adds the dependency. My motivation for the current solution was to be 100% sure we're getting 1. right icons (the original breeze icons repo can change without keeping compatibility), 2. only icons we may ever need (breeze icons repo contains many extra large files).

Would an option, say DISTRO_BUILD or something similar named, be acceptable? Doing something like that:

if(DISTRO_BUILD)

find_package(breeze-icons)
set_target_properties(... TYPE RUNTIME)

else()

fetch & process breez-icons like it is done now

endif()

I'd plan to have the DISTRO_BUILD as the default one and delegate any optimization/fix to the building process of the icons package. E.g. for non-linux it would be built as a minimal file.

Side note: It would be good if distros package breeze icons .rcc file separately from files: breeze-icons recently got support of building the .rcc file. Kexi would not going to use separate (thousands) of icon files anymore...

Unfortunately I can not open that possible fix, due to the following:

You Shall Not Pass: Restricted Maniphest Task
You do not have permission to view this object.
Users with the "Can View" capability:
This object has a custom policy controlling who can take this action.
The owner of a task can always view and edit it.

Possible fix, feel free to test: https://phabricator.kde.org/T3189#45869

Unfortunately I can not open that possible fix, due to the following:
The owner of a task can always view and edit it.

OK, please let me copy the answer:

Would you be able to try kexi.git's 'breeze-icons-check-staniek' branch to see if packager requirements are met? It now depends .rcc file installed by the breeze-icons.

https://phabricator.kde.org/diffusion/KEXI/browse/breeze-icons-check-staniek/

T3189 was filed as a Sysadmin task, and therefore has limited visibility.

Assuming Kexi is now depending on Breeze Icons, and not a copy of Breeze in a separate repository, or any elements it downloads at configure/compile/install time this should be fine from my perspective of the CI system (subject to any build metadata updates to declare breeze-icons a dependency of Kexi if needed)

@bcooksley Yes. Dependencies need to be updated for CI. Is there a way to mark that we require the .rcc file to be built from breeze-icons.git? It's internally controlled by BINARY_ICONS_RESOURCE = ON in that repo.

Also I will probably asking elsewhere: breeze-icons sets set(KF5_VERSION "5.25.0") internally but I can't spot any exported version info that I can use in while depending. I can't see any FindBreezeIcons.cmake so developed some test myself. Ideally I'd like to make BINARY_ICONS_RESOURCE required and make sure the icons are not two years old to avoid misses.

I know our CI actively rebuilds dependencies but others who build Kexi may forget to do that.

By the way, in that branch, existence of breeze-icons.rcc is also checked at Kexi startup now.

@staniek unfortunately not - you'll just have to fail if it isn't available even if you find breeze-icons.

The build metadata doesn't cover detailed settings (and shouldn't need to, it's a rough guideline only)

Possible fix, feel free to test: https://phabricator.kde.org/T3189#45869

Unfortunately I can not open that possible fix, due to the following:
The owner of a task can always view and edit it.

OK, please let me copy the answer:

Would you be able to try kexi.git's 'breeze-icons-check-staniek' branch to see if packager requirements are met? It now depends .rcc file installed by the breeze-icons.

https://phabricator.kde.org/diffusion/KEXI/browse/breeze-icons-check-staniek/

Unfortunately the fix depends on QtWidgets, meaning it wants to connect to an running X sever, which is also something we don't allow during building. Even if we unsandboxed it, it would still break on some dedicated build server, for example.

Possible fix, feel free to test: https://phabricator.kde.org/T3189#45869

Unfortunately I can not open that possible fix, due to the following:
The owner of a task can always view and edit it.

OK, please let me copy the answer:

Would you be able to try kexi.git's 'breeze-icons-check-staniek' branch to see if packager requirements are met? It now depends .rcc file installed by the breeze-icons.

https://phabricator.kde.org/diffusion/KEXI/browse/breeze-icons-check-staniek/

Unfortunately the fix depends on QtWidgets, meaning it wants to connect to an running X sever, which is also something we don't allow during building. Even if we unsandboxed it, it would still break on some dedicated build server, for example.

It uses QIcon which depends on QtGui to be strict, let me check if I can reduce to QGuiApplication. But yes, there's dep on libX11 in QtGui even. Any code that operates of icons or colors do that. I don't know if they connect to X actually. We're not displaying anything here, only loading icon file(s) to see if we have anything working.

@heikobecker could you update the breeze-icons-check-staniek branch? I moved to QGuiApplication.

I ran the branch on openSUSE Build Service, but this resulted in the following error:

[ 194s] No valid breeze-icons.rcc resource file found. The CheckGlobalBreezeIcons
[ 194s] program returned FAILED_TO_RUN. Result: QXcbConnection: Could not connect
[ 194s] to display

This is with the latest breeze-icons from git master.

So it seems that it indeed tries to connect to a Xserver, which of course is not running

So it seems that it indeed tries to connect to a Xserver, which of course is not running

Same here.

@heikobecker @rwooninck

Could you update the breeze-icons-check-staniek branch?
It falls back to QtCore now. Thanks...

Could you update the breeze-icons-check-staniek branch?
It falls back to QtCore now. Thanks...

Unfortunately no time to verify everything, but it builds successfully and I see icons when opening kexi.

Same here. Build is successful, however it should be indicated to the distributions that a change in how the breeze icons are being build is required. This to get the required rcc files,

Thanks a lot, pushing then to master!

staniek added a subscriber: piggz.Jul 23 2016, 9:14 PM
staniek removed a subscriber: staniek.
staniek added a subscriber: staniek.

Same here. Build is successful, however it should be indicated to the distributions that a change in how the breeze icons are being build is required. This to get the required rcc files,

Do you think some update about runtime dependencies in our README.PACKAGERS doc can be enough?
https://quickgit.kde.org/?p=kexi.git&a=blob&f=README.PACKAGERS.md

Or do you mean something more?

staniek commandeered this revision.Jul 23 2016, 9:21 PM
staniek abandoned this revision.
staniek added a reviewer: heikobecker.

Abandoning the patch for other solution already developed.

The discussed fix is in kexi.git master branch now. Feel free to test.