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

Install qt5 soundsources to a different folder to avoid crashes, fixes lp1774639 #1698

Merged
merged 2 commits into from
Nov 4, 2018

Conversation

daschuer
Copy link
Member

@daschuer daschuer commented Jun 1, 2018

https://bugs.launchpad.net/mixxx/+bug/1774639

This fixes the bug for Linux. I think there is something missing for the Windows and Mac OS installer.

@daschuer daschuer changed the base branch from master to 2.1 June 1, 2018 20:24
@daschuer
Copy link
Member Author

daschuer commented Jun 7, 2018

It is really annoying to have a crash every time I change from QT4 to 5 and back.
This PR solves this. Can we merge it, at least as an intermediate solution?

@daschuer
Copy link
Member Author

daschuer commented Jun 7, 2018

Now it fixes also https://bugs.launchpad.net/mixxx/+bug/1711173
The installations for mac OS and Windows are still missing.
Can one explain how to make it QT5 conditional?

@Be-ing
Copy link
Contributor

Be-ing commented Jun 11, 2018

This feels like a hacky solution. Why isn't the old plugin overwritten by the new one?

@Be-ing
Copy link
Contributor

Be-ing commented Jun 11, 2018

Maybe this bug is related?

@daschuer
Copy link
Member Author

It is not related. The number of soundsources depends on the compile options. So it may happen, that the user has installed a build with more soundsources. These will stick in these folders as intended.

An other issue is that the user might use Mixxx from the build tree, which used a different qt version than the original installed one. In this case Mixxx tries to load the plug-ins from the install path as well. (My case)

@daschuer
Copy link
Member Author

For this PR we need to test and probably fix the windows and Mac installer. Who is able make the installers Qt version aware? Alternative, we can ignore the issue and install master alwaxs to qt5 and 2.1 always to qt4 path.

@daschuer daschuer added this to the 2.2.0 milestone Jun 27, 2018
@daschuer
Copy link
Member Author

daschuer commented Jul 6, 2018

@sblaisot:What is your opinion here? Is it difficult to make the install location qt version aware on windows?

@sblaisot
Copy link
Member

sblaisot commented Aug 15, 2018

The installer already takes whatever subdir exist in build_dir so if the subdir is called pluginsqt5 instead of plain plugins it will take that and install as-is on target machine: https://github.com/mixxxdj/mixxx/blob/master/src/SConscript#L712,L716

So all you have to do is to build in the right sudbir here:
https://github.com/mixxxdj/mixxx/blob/master/src/SConscript#L567,L570

You already have an example of qt4/5 logic here:
https://github.com/mixxxdj/mixxx/blob/master/src/SConscript#L328,L343

@@ -51,6 +51,22 @@ const QStringList SOUND_SOURCE_PLUGIN_FILENAME_PATTERN; // empty
#endif

QList<QDir> getSoundSourcePluginDirectories() {

Copy link
Member

@rryan rryan Nov 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on macOS the bundle plugin directory must be named PlugIns:
https://developer.apple.com/library/archive/documentation/CoreFoundation/Conceptual/CFBundles/BundleTypes/BundleTypes.html#//apple_ref/doc/uid/10000123i-CH101-SW1

macOS is unaffected by this bug because .app bundles can't get files mixed up so I would just leave the __APPLE__ codepaths unchanged

@rryan
Copy link
Member

rryan commented Nov 2, 2018

I think Windows is mostly unaffected by this bug because the WiX installer will not install Mixxx on top of an old install -- it requires you to run the uninstaller first, which will delete the plugins.

Right @sblaisot ?

@daschuer daschuer changed the base branch from 2.1 to 2.2 November 3, 2018 19:29
@daschuer daschuer changed the base branch from 2.2 to 2.1 November 3, 2018 19:31
@daschuer
Copy link
Member Author

daschuer commented Nov 3, 2018

Ok, now it works for Linux only. No crash anymore.
Linux uses now:

/usr/local/lib/mixxx/plugins/soundsourceqt5/
/usr/lib/mixxx/plugins/soundsourceqt5/
~/.mixxx/plugins/soundsourceqt5/

all other platforms are not effected.

We "only" have to change:
https://www.mixxx.org/wiki/doku.php/add-ons
the manual is not effected.

The linux packagers are also not effected because at least for ubuntu they do not ship soundsource plugins.

@mixxxdj mixxxdj deleted a comment Nov 3, 2018
@mixxxdj mixxxdj deleted a comment Nov 3, 2018
@mixxxdj mixxxdj deleted a comment Nov 3, 2018
@mixxxdj mixxxdj deleted a comment Nov 3, 2018
@mixxxdj mixxxdj deleted a comment Nov 3, 2018
@mixxxdj mixxxdj deleted a comment Nov 3, 2018
@daschuer
Copy link
Member Author

daschuer commented Nov 4, 2018

re-added the vamp fix

@rryan rryan changed the base branch from 2.1 to 2.2 November 4, 2018 18:14
@daschuer
Copy link
Member Author

daschuer commented Nov 4, 2018

The base 2.1 is correct. It does not change anything in a qt4 2.1 build, but if one wants to try qt5 it will work without a crash.

@daschuer daschuer changed the base branch from 2.2 to 2.1 November 4, 2018 20:27
@daschuer
Copy link
Member Author

daschuer commented Nov 4, 2018

I have tested both.

@daschuer
Copy link
Member Author

daschuer commented Nov 4, 2018

ready ..

@rryan
Copy link
Member

rryan commented Nov 4, 2018

cool, thanks

@rryan rryan merged commit 67af09b into mixxxdj:2.1 Nov 4, 2018
@daschuer daschuer deleted the soundsourceqt5 branch September 7, 2021 21:08
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.

4 participants