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

Fix build with Qt 5.11 beta3 (drop qt5_use_modules) #36

Merged
merged 3 commits into from Jul 25, 2019

Conversation

@a17r
Copy link

commented Apr 15, 2018

qt5_use_modules is gone in Qt 5.11_beta3 which breaks liblastfm.

@a17r a17r force-pushed the a17r:qt-5.11 branch from 14beb54 to aeb0cbc Apr 17, 2018

@krop

This comment has been minimized.

Copy link

commented May 24, 2018

Cleanup include dirs looks wrong.
Qt5::Core Qt5::Network Qt5::Xml are link targets which makes 'include_directories(...)' useless. You can't use it that way.

@krop

This comment has been minimized.

Copy link

commented May 24, 2018

+1 for the other commits.

@ilovezfs

This comment has been minimized.

Copy link

commented Jun 7, 2018

@a17r any updates here?

@a17r a17r force-pushed the a17r:qt-5.11 branch from aeb0cbc to 7453db4 Jun 14, 2018

@a17r

This comment has been minimized.

Copy link
Author

commented Jun 14, 2018

Cleanup include dirs looks wrong.

@krop It appears we do not need it anyway, so I dropped it.

@ilovezfs

This comment has been minimized.

Copy link

commented Jul 11, 2018

@ben-xo @farragar @jammus can this be merged?

@ben-xo

This comment has been minimized.

Copy link
Member

commented Jul 13, 2018

I have no real way to test this. Any changelog entries or perhaps a summary of what the changes in the PR do?

Does it still work with Qt4 as well?

@a17r

This comment has been minimized.

Copy link
Author

commented Aug 22, 2018

or perhaps a summary of what the changes in the PR do?

I like to think the git messages tell that story well. Slightly confused as I don't see a ChangeLog file in the project.

Does it still work with Qt4 as well?

Yes it should still work fine with Qt4, but since Qt4 has been removed from our repositories I have no way to test this anymore. (thinking I did test it at the time of writing the changes)

@a17r

This comment has been minimized.

Copy link
Author

commented Sep 12, 2018

@ben-xo incidentally, I found a system with Qt4 still installed, and was able to build this successfully with Qt4 enabled.

@a17r a17r force-pushed the a17r:qt-5.11 branch from 7453db4 to 23fb450 Nov 21, 2018

@a17r

This comment has been minimized.

Copy link
Author

commented Jul 24, 2019

ping

@ben-xo

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

Hi @a17r - sorry there's been no action here. This library is essentially legacy / unmaintained at the moment.

I feel weird merging something that's not tested in-house to master, but I could merge it to a qt5 branch? Not sure what to suggest.

@krop

This comment has been minimized.

Copy link

commented Jul 24, 2019

@ben-xo tested in-house by who ? you're the only one that reacted to this pull request and distributions are using these changes for more than a year now.

@a17r

This comment has been minimized.

Copy link
Author

commented Jul 24, 2019

I feel weird merging something that's not tested in-house to master, but I could merge it to a qt5 branch? Not sure what to suggest.

The changes are rather straightforward and in cmake only, no need for a branch.

Removing all traces of Qt4 on the other hand could be done in a separate branch, or conversely, a qt4 branch with the current state to proceed with cleanup in master.

@ben-xo
Copy link
Member

left a comment

@ben-xo tested in-house by who ?

Well yes, exactly; there is nobody in-house to test it. But that doesn't contradict how I feel about pushing the change to master.

liblastfm is primarily a component of the Last.fm Desktop Client, so that is the benchmark for a successful test; and right now I don't have a set-up that can prove it will, as the build pipeline is pretty ancient.

you're the only one that reacted to this pull request and distributions are using these changes for more than a year now.

I would be more than happen to suggest an administrative solution here, such as:

  • delegating a new maintainer (fork optional)
  • merging this (and future compatibility changes) somewhere new

I'm also happy to listen to other suggestions.

@ben-xo

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

Removing all traces of Qt4 on the other hand could be done in a separate branch, or conversely, a qt4 branch with the current state to proceed with cleanup in master.

This seems like a good option.

@ben-xo ben-xo merged commit f867df5 into lastfm:master Jul 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.