-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 on Ubuntu caused by incorrect order of libraries #1926
Conversation
Should I squash these two commits into one? |
I don't mind in this case. Normally you should not rewrite the history in a pending PR. |
Is the 2.1 branch also effected? |
The first dl is added here: Line 363 in 061073f
Can we remove it? |
No, this bug was caused by #1841 which was merged to master. |
@daschuer Hmm, if we remove |
I tested these changes on Linux Mint 18.3 and Fedora 29 and it seems that duplicate |
Please check if the original issue was caused by a double -lvamp-hostsdk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
conf.CheckLib(['dl', 'libdl']) already adds -ldl.
The double -ldl should be fixed before merge.
I can confirm that this fix error https://bugs.launchpad.net/mixxx/+bug/1804411 on Linux Mint 19 x64 But if I remove the line suggested by @daschuer in #1959 I get the following error:
but If I remove
so there might be more to do.. |
thanks! |
OK I have managed to resolve duplicates. They were caused by having two VAMP SDKs: one installed on system and the other bundled with Mixxx. I have tested this only on my Linux Mint 18.3 system, on which Mixxx is always taking the bundled VAMP SDK. Can you test this on Linux Mint 19 as well as other distros? |
@nopeppermint Glad to hear it 😄 |
Greate, no it is fine, Thank you. I think just need to comment why we need a special treatment here. |
conf.CheckLib(['dl', 'libdl']) | ||
# Note: We are adding -ldl to LIBS in sources(), so | ||
# don't add it now in order to prevent duplication. | ||
conf.CheckLib(['dl', 'libdl'], autoadd=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# we cant add it here,because it need to be added after vamp-hostsdk. See https://bugs.launchpad.net/mixxx/+bug/1804411
@@ -383,7 +387,12 @@ def sources(self, build): | |||
env.SConscript(env.File('SConscript', vamp_dir)) | |||
|
|||
build.env.Append(LIBPATH=self.INTERNAL_VAMP_PATH) | |||
build.env.Append(LIBS=['vamp-hostsdk']) | |||
|
|||
build.env.Append(LIBS=['vamp-hostsdk']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like this:
# we add this here unconditionally because it is expected by vamp-plugins/SConscript which does further adjustments
Thank you, for your patience. LGTM. |
Thank you for merging. |
This fixes https://bugs.launchpad.net/mixxx/+bug/1804411
The solution is to specify
-ldl
after-lvamp-hostsdk
. However, I couldn't find a way to remove the first-ldl
, so-ldl
is currently duplicated.