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

Ensure library link flags are unique, fixing build for Wasm/webR #152

Closed
wants to merge 1 commit into from

Conversation

georgestagg
Copy link

Fixes compiling with Emscripten for webR, where linking is static and so multiple instances of the same library link flag, such as -lz, leads to duplicate symbol errors.

Fixes compiling with Emscripten for webR, where linking is static and
so multiple instances of the same library link flag, such as `-lz`,
leads to duplicate symbol errors.
@jeroen
Copy link
Member

jeroen commented Dec 13, 2023

I think this is not a bug in the package configure but the build tooling. The package simply forwards the flags from pkg-config to the linker. Lots of packages will have this issue. Perhaps we should shim pkg-config to filter duplicates.

@georgestagg
Copy link
Author

Perhaps we should shim pkg-config to filter duplicates.

Yes, good idea. As discussed, we should indeed handle this in r-wasm/rwasm. That also means our tweaks will only take effect for Emscripten builds, which is a nice safety net.

The package simply forwards the flags from pkg-config to the linker

I don't think that's strictly true here. Even after shimming pkg-config I'm getting a duplicate symbol error, I think due to the extra -ljpeg added at the end here:

ragg/configure

Lines 37 to 39 in 792c1e1

echo "Found pkg-config cflags and libs!"
PKG_CFLAGS=${PKGCONFIG_CFLAGS}
PKG_LIBS="${PKGCONFIG_LIBS} -ljpeg"

@jeroen
Copy link
Member

jeroen commented Dec 13, 2023

Ah you are right. I think the problem was actually introduced recently here: fa87a16

Previously we hardcoded libjpeg libs as -ljpeg because it did not used to have a pc file. But then fa87a16 was added, so that results in duplicates and also the configuration failing on systems without a jpeg.pc.

I think fa87a16 was a mistake and should better be reverted, or otherwise we can remove the hardcoded -ljpeg.

@georgestagg
Copy link
Author

I'm going to close this in PR favour of r-wasm/rwasm#9, but we should continue the discussion about what to do with -ljpeg. So, I'll open an issue instead and link it here.

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.

2 participants