Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce source incompatibility against last version #86

Closed
wants to merge 1 commit into from

Conversation

aleixpol
Copy link
Collaborator

@aleixpol aleixpol commented Nov 4, 2016

Makes it possible to have something like this:
https://phabricator.kde.org/D3265

@ximion
Copy link
Owner

ximion commented Nov 4, 2016

Is this really not solveable in the Discover branch is a reasonable way? (esp. the enum aliasing...)

@ximion
Copy link
Owner

ximion commented Nov 5, 2016

At Debian we are using this patch to build with more recent AppStream releases: https://anonscm.debian.org/git/pkg-kde/plasma/plasma-discover.git/tree/debian/patches/01_new-appstreamqt.patch
So far this seems to work well, but I haven't tested yet if this also works with older AS versions (otherwise I would have submitted it for review).
The patch is based from the one you have under review at Phab at time.

@aleixpol
Copy link
Collaborator Author

aleixpol commented Nov 8, 2016

Closing and moving the ifdefing to Discover.

@aleixpol aleixpol closed this Nov 8, 2016
@aleixpol aleixpol reopened this Nov 8, 2016
@aleixpol
Copy link
Collaborator Author

aleixpol commented Nov 8, 2016

Wait, we'll still need something to find the AppStreamConfig.cmake, do you also want us to try both?

@ximion
Copy link
Owner

ximion commented Nov 8, 2016

In the patch linked above I simply tried both and emitted an error if none was found.
Not super pretty, but effective.

I very much want to get rid of the old name and API, if we re-add this to AppStream now, this means we will need to support this for a very long time again, if we don't want to break older Discover versions. By not reintroducing it, we can be sure Discover uses the new API and names when linked to the latest AppStream lib.

@ximion ximion closed this Nov 8, 2016
@rdieter
Copy link

rdieter commented Nov 11, 2016

fwiw, I tried using the aforementioned debian patch on my fedora box... and plasma discover crashes pretty soon after browsing packages for a minute or 2.

@ximion
Copy link
Owner

ximion commented Nov 11, 2016

Thanks for the hint - did you try that with the current AppStream version?
I'll try a longer-running Plasma Discover session today, maybe I can reproduce this issue.

@aleixpol
Copy link
Collaborator Author

I pushed it to Plasma/5.8, please test that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants