Is linking juce modules with PUBLIC visibility dangerous?

[chrhaase]

I just fell over this:

Due to the way that INTERFACE libraries work in CMake, linking to a module added in this way must be done using PRIVATE visibility. Using PUBLIC will cause the module sources to be added both to the target's SOURCES and INTERFACE_SOURCES, which may result in many copies of the module being built into a single target, which would cause build failures in the best case and silent ODR violations in the worst case. Scary stuff!

So might be a bit dangerous to go

target_link_libraries("${PROJECT_NAME}"
    PRIVATE
    Assets
    PUBLIC
    juce::juce_audio_utils
    juce::juce_audio_processors
    juce::juce_recommended_config_flags
    juce::juce_recommended_lto_flags
    juce::juce_recommended_warning_flags)

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? :)

[sudara]

Hmmm yeah, this reminds me of 59c9872

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...

[chrhaase]

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)

[sudara]

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!

[chrhaase]

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

[sudara]

Ok thanks, that's helpful!!

[chrhaase]

Have you seen it before?

[sudara]

No, haven't seen it, but I'll try to do some reproducing, so it's useful <3

[sudara]

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 171731a 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!

[sudara]

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....

[sudara]

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

[chrhaase]

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..

[faressc]

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...

[sudara]

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

1. You had the intuition to create a library that "has all the shared code" - it reminded me that that's already the function/intention of ${PROJECT_NAME} (the juce shared code target)
2. This shared code target has lots of visibility wrangling going on behind the scenes, like the re-exporting of private modules as interface.

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...

[sudara]

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.

[sudara]

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...

[sudara]

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?

[faressc]

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...

[sudara]

I still get the nasty warnings for my external libraries

Are you also linking to those with PRIVATE visibility?

[faressc]

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..

[sudara]

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:

1. why that works
2. if there's ODR violations/warnings why I'm not seeing them here...
3. assuming it's problematic, what are the alternatives (going the @chrhaase INTERFACE route, what else...?)

[sudara]

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?...

[sudara]

Here's what I'm working with: https://github.com/sudara/pamplejuce/compare/shared_code_interface?expand=1

[sudara]

Whups, got it! target_sources needed to have INTERFACE visibility :D

[chrhaase]

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?

[chrhaase]

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!

[sudara]

Awesome that sounds promising! I’ll try it out on my main project here too.

[sudara]

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

[chrhaase]

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 :)

[sudara]

sorry, i meant inside of your own juce modules, not the official ones… seeing some discrepancies with some warnings not showing up there…

[chrhaase]

Ah, I think I understand. So some warnings disappeared after changing to this approach?

[sudara]

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:

• Compiler differences (people were sending me MSVC warnings)
• CLion's clang-tidy showing me some OTHER set of warnings in the UI (for some reason I always think of those as compiler warnings, but they are separate)
• Me not having a good workflow for dealing with compiler warnings in general (I've since added some stuff to the inspector CI to help)

I wrote more here: https://forum.juce.com/t/not-seeing-warnings-in-juce-modules/57472

[sudara]

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>)

[sudara]

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.