Closed chrhaase closed 1 year ago
Hmmm yeah, this reminds me of https://github.com/sudara/pamplejuce/commit/59c98720811060479a3bbc266a5ade7a2319c32c
See https://forum.juce.com/t/windows-linker-issue-on-develop/55524/2
Deep in a bunch of failing tests right now, but 100% with you that something's fishy...
I see.
I've been looking a bit more on this now and maybe the "stealing" approach might not be for me. I have a setup where I have a MY_PROJECT_UNIT_TESTS
defined in my test target, so I can easily embed unit tests in source files. I've been ignoring some sketchy linker warnings for a while, which are probably actually ODR violations and probably caused by the shared plugin lib being compiler both with the flag and without. Not sure about it.
Anyway, the solution I'm trying out now is just creating an interface library that has all the shared code. To me this is easier to comprehend and the scary linker warnings are gone. Still not 100% on this, but this is roughly what I'm doing: (still based on pamplejuce)
add_library(SharedCode INTERFACE)
# Manually list all .h and .cpp files for the plugin
set(SourceFiles
src/PluginEditor.h
src/PluginProcessor.h
src/PluginEditor.cpp
src/PluginProcessor.cpp)
target_sources(SharedCode INTERFACE ${SourceFiles})
target_link_libraries(SharedCode INTERFACE
juce_audio_utils
juce_audio_devices
my_custom_juce_module
some_third_party_lib
juce::juce_recommended_config_flags
juce::juce_recommended_lto_flags
juce::juce_recommended_warning_flags)
target_compile_definitions(SharedCode INTERFACE
GLISS_DEV_MODE=<${GLISS_DEV_MODE}>
# JUCE_WEB_BROWSER and JUCE_USE_CURL would be on by default, but you might not need them.
JUCE_WEB_BROWSER=0 # If you remove this, add `NEEDS_WEB_BROWSER TRUE` to the `juce_add_plugin` call
JUCE_USE_CURL=0 # If you remove this, add `NEEDS_CURL TRUE` to the `juce_add_plugin` call
JUCE_VST3_CAN_REPLACE_VST2=0)
target_link_libraries("${PROJECT_NAME}" PRIVATE SharedCode)
# Test target
target_compile_definitions(Tests PRIVATE GLISS_UNIT_TESTS=1)
# Our test executable also wants to know about our plugin code...
target_include_directories(Tests PRIVATE ${SourceDir})
target_link_libraries(Tests PRIVATE
${PROJECT_NAME} # If this is removed build fails with "error: use of undeclared identifier 'JucePlugin_Name'"
SharedCode
Catch2::Catch2)
I've been ignoring some sketchy linker warnings for a while
Do you happen to have an example of those warnings laying around?
Not sure I've seen anything suspicious from the linker recently, but I haven't been paying too close attention since resolving those last issues in the juce thread.
Oh, and thanks for sharing your approach!
I don't one from my project right now - which is also fortunate :) - but it's this pattern:
ld: warning: direct access in function [...] to global weak symbol [...] means the weak symbol cannot be overridden at runtime. This was likely caused by different translation units being compiled with different visibility settings.
Might be completely unrelated, though - I'm not really sure what's causing it
Ok thanks, that's helpful!!
Have you seen it before?
No, haven't seen it, but I'll try to do some reproducing, so it's useful <3
Just to followup now that I've had time to look into this....
At least in the case of a plugin, the juce target (in pamplejuce's case "${PROJECT_NAME}"
is actually a shared interface target (not the end result executable plugin targets) so I think the solution here for pamplejuce is to have the juce dependencies be INTERFACE
visibility and for the test target to grab the COMPILE_DEFINITIONS
and INCLUDE_DIRECTORIES
from it.
I changed the visibility away from PUBLIC
to INTERFACE
in https://github.com/sudara/pamplejuce/commit/171731a9b91057979ebd53f73d9acf3cf14aa6b2 and everything seems happy!
I'll tag @reuk just in case — he helped me out on the ODR issues originally and might be able to point out any further wrongdoing!
Hmm, it looks like non-juce dependencies, such as my own modules or third party dependencies like nlohmann_json::nlohmann_json
or tomlplusplus::tomlplusplus
have to be PUBLIC
? They don't seem happy as INTERFACE
....
Hmm, seeing multiple definition linker errors with the Linux build (and only the linux build?) so maybe it's back to square 1 here https://github.com/sudara/pamplejuce/actions/runs/5391280333/jobs/9790971356
(.text+0x0): multiple definition of `juce::detail::ScopedContentSharerInterface::shareImages(juce::Array<juce::Image, juce::DummyCriticalSection, 0> const&, std::unique_ptr<juce::ImageFileFormat, std::default_delete<juce::ImageFileFormat> >, juce::Component*)'; CMakeFiles/Pamplejuce_VST3.dir/JUCE/modules/juce_gui_basics/juce_gui_basics.cpp.o (symbol from plugin):(.text+0x0): first defined here
At least in the case of a plugin, the juce target (in pamplejuce's case "${PROJECT_NAME}" is actually a shared interface target (not the end result executable plugin targets)
I think it's actually a static lib, not interface: docs. So I would think that PRIVATE
is the way to go - also for the other dependencies you're mentioning?
Weird that it's only Linux that fails, right now..
Hi there, I have been following this issue and I wanted to share my experience.
I get the same warnings as @chrhaase, for my PluginProcessor and PluginEditor classes as well as the classes from external Libraries of the type
ld: warning: direct access in function [...] to global weak symbol [...] means the weak symbol cannot be overridden at runtime. This was likely caused by different translation units being compiled with different visibility settings.
when I link the libraries in the following way:
juce_add_plugin(${PROJECT_NAME}{...}
target_sources(${PROJECT_NAME} PRIVATE ${SOURCES} ...)
target_compile_definitions(${PROJECT_NAME}
PUBLIC
# JUCE_WEB_BROWSER and JUCE_USE_CURL would be on by default, but you might not need them.
JUCE_WEB_BROWSER=0 # If you remove this, add `NEEDS_WEB_BROWSER TRUE` to the `juce_add_plugin` call
JUCE_USE_CURL=0 # If you remove this, add `NEEDS_CURL TRUE` to the `juce_add_plugin` call
...
)
target_link_libraries(${PROJECT_NAME}
PUBLIC
BinaryData
juce::juce_audio_utils
juce::juce_dsp
...
external_library
PUBLIC
juce::juce_recommended_config_flags
juce::juce_recommended_lto_flags
juce::juce_recommended_warning_flags
)
add_executable(Test ${TestFiles})
target_link_libraries(Test PRIVATE gtest_main benchmark::benchmark "${PROJECT_NAME}")
On the other hand when I use the approach of the current pamplejuce version with
target_link_libraries(${PROJECT_NAME}
PRIVATE
BinaryData
INTERFACE
juce::juce_audio_utils
juce::juce_dsp
...
external_library
PUBLIC
juce::juce_recommended_config_flags
juce::juce_recommended_lto_flags
juce::juce_recommended_warning_flags
)
The PluginProcessor and PluginEditor classes will not find the juce modules, which might make sense because when linking with INTERFACE the libraries are not added to the current target.
With the INTERFACE library approach from @chrhaase:
juce_add_plugin(${PROJECT_NAME}{...}
add_library(SharedLibs INTERFACE)
target_sources(SharedLibs INTERFACE ${SOURCES} ...)
target_compile_definitions(SharedLibs
INTERFACE
# JUCE_WEB_BROWSER and JUCE_USE_CURL would be on by default, but you might not need them.
JUCE_WEB_BROWSER=0 # If you remove this, add `NEEDS_WEB_BROWSER TRUE` to the `juce_add_plugin` call
JUCE_USE_CURL=0 # If you remove this, add `NEEDS_CURL TRUE` to the `juce_add_plugin` call
...
)
target_link_libraries(SharedLibs
INTERFACE
BinaryData
juce::juce_audio_utils
juce::juce_dsp
...
external_library
juce::juce_recommended_config_flags
juce::juce_recommended_lto_flags
juce::juce_recommended_warning_flags
)
target_link_libraries(${PROJECT_NAME}
PRIVATE
SharedLibs
)
add_executable(Test ${TestFiles})
target_link_libraries(Test
PRIVATE
gtest_main
benchmark::benchmark
SharedLibs
"${PROJECT_NAME}")
finally all the warnings in the PluginProcessor and PluginEditor classes are gone. But I still get these warnings for the external_library:
ld: warning: direct access in function [...] to global weak symbol [...] means the weak symbol cannot be overridden at runtime. This was likely caused by different translation units being compiled with different visibility settings.
When I add
set(CMAKE_CXX_VISIBILITY_PRESET hidden)
set(CMAKE_VISIBILITY_INLINES_HIDDEN ON)
at the top of my CMakeLists.txt the warnings are gone finally.
As the warning suggests it is "likely caused by different translation units being compiled with different visibility settings", which might (but must not) result from ODR violation issues. Therefore I assume the warnings for the PluginProcessor/Editor classes were triggered because the JUCE_DEPENDENCIES were linked separately to the plugin target and the test target, which resulted in those different visibilities, and which got fixed with the INTERFACE library approach. The warnings from the external libraries on the other hand might be triggered by real "different visibility issues", which we could fix with the two lines above.
Wow, this has gotten way too long but I hope that you can follow me... Ohh and don't get confused I use googletest and benchmark and not catch2 but essentially its the same...
I think it's actually a static lib, not interface
Ahhh, I forgot that INTERFACE
can be a target type, not just a visibility setting yells in cmake 🙈
What I was (poorly) trying to say was ${PROJECT_NAME}
is already a library that the end result plugin targets (i.e. AU/VST3 targets) link to.
The reason I wanted to point it out is because
${PROJECT_NAME}
(the juce shared code target) Anyway, this is all at the limits of my CMake understanding, that's why I was hoping @reuk might have some recommendations. It's possible @benthevining would have opinions here too...
I get the same warnings
This is interesting to me. I couldn't reproduce the original linker warning, but I'll give it another try!
set(CMAKE_CXX_VISIBILITY_PRESET hidden)
set(CMAKE_VISIBILITY_INLINES_HIDDEN ON)
These are above my pay grade.... I wouldn't know how to judge if it's the "right" solution or if the issue is just being masked.
Also just to try to get clear, @chrhaase's original concern was this line from JUCE's CMake API docs:
linking to a module added in this way must be done using PRIVATE visibility
Emphasis mine: this is in the juce_add_module
section, so I read this warning as: "When you call juce_add_module
, it sets up an interface target that you must later link as PRIVATE
"
In the case of plain pamplejuce, juce_add_module
isn't being used at all...
However, in my real projects i have like 15 calls to juce_add_module
, so I'm motivated to figure this out clearly...
Ok, I'm not seeing any warnings or ODL issues locally or in CI building with the following:
target_link_libraries("${PROJECT_NAME}"
PRIVATE
Assets
juce::juce_audio_utils
juce::juce_audio_processors
[my third party modules]
PUBLIC
juce::juce_recommended_config_flags
juce::juce_recommended_lto_flags
juce::juce_recommended_warning_flags)
and the Tests target "stealing" the compile defs from the shared code target:
target_compile_definitions(Tests PRIVATE $<TARGET_PROPERTY:${PROJECT_NAME},COMPILE_DEFINITIONS>)
target_include_directories(Tests PRIVATE $<TARGET_PROPERTY:${PROJECT_NAME},INCLUDE_DIRECTORIES>)
However, on my "real" projects I was doing something like
target_compile_definitions(Tests PRIVATE
JUCE_MODAL_LOOPS_PERMITTED=1 # let us run Message Manager in tests
RUN_MELATONIN_TESTS=1 # run tests in modules
)
which is no longer working.... so I see the motivation behind the INTERFACE target.... wondering if there's a way to have my cake and eat it too... from what I understand wanting different compile definitions defeats the purpose of having a shared compiled library — we essentially want two different compiled targets depending on whether it's the main app or tests.
@faressc would you mind giving this config a go?
I just tried this one and indeed it is working just fine in my project! :) After changing from INTERFACE to PRIVATE the Test target could steal successfully the compile refs and include dirs. I still get the nasty warnings for my external libraries, but I can get rid of them with the visibility lines...
I still get the nasty warnings for my external libraries
Are you also linking to those with PRIVATE visibility?
Yes but I believe those warnings are caused by the libraries I use. In the google benchmark lib the visibility settings are set to set(CMAKE_CXX_VISIBILITY_PRESET hidden) set(CMAKE_VISIBILITY_INLINES_HIDDEN ON)
have a look here in line 45-46. So my guess is this has nothing to do with ODR violations... But yeah its just a guess, I am not a fully proofed CMake expert..
Ok cool — so officially the reason the juce modules were PUBLIC in pamplejuce is because somehow that makes it possible to add compile flags to only the test target. But i have no idea:
I'm coming back to this, as I have the same needs as @chrhaase re: adding compile defs to only the test target.
It really does seem like the only two options are an INTERFACE target or manually passing flags/options (like RUN_MY_TESTS
) into CMake, which seems less desirable.
However, I'm having trouble going the INTERFACE target route, seeing this error on compile of a plugin wrapper target:
Undefined symbols for architecture arm64:
"createPluginFilter()", referenced from:
juce::createPluginFilterOfType(juce::AudioProcessor::WrapperType) in juce_audio_plugin_client_AU_1.mm.o
I'm assuming the juce_add_plugin call is still being called on PROJECT_NAME (which sets up the juce shared code static library) right above the code posted, so I'm kind of confused where I'm going wrong. It almost looks like the plugin wrappers can't find my code, which is strange!
Also I'm a bit confused by the target_link_libraries
in @chrhaase's example:
target_link_libraries(Tests PRIVATE
${PROJECT_NAME} # If this is removed build fails with "error: use of undeclared identifier 'JucePlugin_Name'"
SharedCode
Catch2::Catch2)
By linking to both PROJECT_NAME and SharedCode, isn't the SharedCode target being linked to twice?...
Here's what I'm working with: https://github.com/sudara/pamplejuce/compare/shared_code_interface?expand=1
Whups, got it! target_sources
needed to have INTERFACE visibility :D
Also I'm a bit confused by the
target_link_libraries
in @chrhaase's example:
Yes, I am a bit confused about this, too! And a bit worried, actually. I'll take a look at my own plugin project tonight and get back!
Even if it works I think it's kinda smelly. Maybe there's a cleaner solution. Maybe even stealing just the compile defs from the {PROJECT_NAME}
instead of linking to it?
Hmm, I'm more and more convinced that linking tests to the static lib ${PROJECT_NAME}
is not what we want here. After all, it's compiled with the wrong preprocessor defs (from test target's point of view). We only want to add the compile definitions that the juce_add_plugin
function adds (i.e. JucePlugin_Name etc.
, so we can use our plugin processor in the tests.
I just tried adding this target_compile_definitions(SharedCode INTERFACE $<TARGET_PROPERTY:${PROJECT_NAME},COMPILE_DEFINITIONS>)
(stealing the compile defs) and removing ${PROJECT_NAME}
from the target_link_libraries(Tests ...)
call. It works in my own project. Will try on pamplejuce tomorrow!
Awesome that sounds promising! I’ll try it out on my main project here too.
Hmm, was wondering, should this set compile defs on the Tests
target (or does it need to be the SharedCode target?).
target_compile_definitions(SharedCode INTERFACE $<TARGET_PROPERTY:${PROJECT_NAME},COMPILE_DEFINITIONS>)
Also, are you seeing juce warnings inside of juce modules? Right now it seems a bit sketchy for me, wondering if I'm doing something wrong with juce::juce_recommended_warning_flags
Hmm, yeah that does seem sketchy. Definitely seems more right to just add those to the Tests
target!
Also, are you seeing juce warnings inside of juce modules? Right now it seems a bit sketchy for me, wondering if I'm doing something wrong with juce::juce_recommended_warning_flags
What do you mean by 'inside of juce modules'? I'm seeing warnings from my own juce modules that I need to fix, but that is expected :)
sorry, i meant inside of your own juce modules, not the official ones… seeing some discrepancies with some warnings not showing up there…
Ah, I think I understand. So some warnings disappeared after changing to this approach?
Yeah, I thought I was having trouble with warnings with this approach, because people have been sending me PRs on my modules.
But actually, I think the problem was:
I wrote more here: https://forum.juce.com/t/not-seeing-warnings-in-juce-modules/57472
should this set compile defs on the Tests target?
This seems to work with visibility PRIVATE
target_compile_definitions(Tests PRIVATE $<TARGET_PROPERTY:${PROJECT_NAME},COMPILE_DEFINITIONS>)
Thanks for all the help on this! I feel much better about it. I think I will also spend a bit of time to break apart the CMake into a few include files, to help clean up the main file.
I also want to see if all the Xcode specific stuff is still necessary or if JUCE improved support.
I just fell over this:
So might be a bit dangerous to go
Or what? Changing to
PRIVATE
of course breaks the test target further down. Was just wondering if you were aware of this and what your thoughts are, @sudara? :)