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

Use SHADERC_BUILD_SHARED to control static/shared library #498

Closed
wants to merge 1 commit into from

Conversation

antiagainst
Copy link
Contributor

Previously we build both static and shared library at the same
time. Because of that, we cannot name both the static and shared
library as shaderc.

This commit stops compiling both static and shared library.
Instead we let the user choose now.

Previously we build both static and shared library at the same
time. Because of that, we cannot name both the static and shared
library as shaderc.

This commit stops compiling both static and shared library.
Instead we let the user choose now.
@antiagainst antiagainst self-assigned this Oct 3, 2018
@antiagainst
Copy link
Contributor Author

I'd suggest to turn on "Hide whitespace changes" in "Diff settings" when doing the review.

@dneto0
Copy link
Collaborator

dneto0 commented Oct 4, 2018

This will break downstream projects?

@antiagainst
Copy link
Contributor Author

You mean renaming the shared library name or not building static and shared libraries together?

@dneto0
Copy link
Collaborator

dneto0 commented Oct 4, 2018

I mean renaming the library name.

@antiagainst
Copy link
Contributor Author

I checked the latest Vulkan SDK, only shaderc_combined.a is shipped. So won't be a problem for users replying on the SDK. Then for others that built Shaderc manually, I think they need to periodically accommodate the changes from Shaderc anyway?

We can create symbolic links if necessary.

@antiagainst
Copy link
Contributor Author

@dneto0: thoughts?

@jeeb
Copy link

jeeb commented Nov 12, 2018

As a 3rd party stand-byer if this helps with the fact that:

  1. A project currently has to be on the look-out for shared and static shaderc with separate names.
  2. Now there can be just a single .pc file instead of two (one for shaderc-static, one for shaderc-shared).

I am all for this.

@haasn
Copy link

haasn commented Nov 12, 2018

Now there can be just a single .pc file instead of two (one for shaderc-static, one for shaderc-shared).

(disregarding that shaderc does not have a .pc file yet anyway)

@eli-schwartz
Copy link

CMake has this builtin. Just don't specify whether it is STATIC or SHARED, and use the toggle described in https://cmake.org/cmake/help/latest/variable/BUILD_SHARED_LIBS.html to allow CMake to automatically choose the preferred library type.

In addition to being less work, this means there is a common interface for this setting that people can utilize across many different CMake projects. You can add it as a default-off option() to make it user-visible, though.

Then just check the current value of it to determine whether to do the *_combined stuff.

@antiagainst
Copy link
Contributor Author

@eli-schwartz: As I commented in the source code:

# Note that we are not using or setting CMake BUILD_SHARED_LIBS here since that
# also affects external projects and makes libshaderc.so to depend on shared
# libraries compiled from external projects. We want libshaderc.so to depend
# on only standard system libraries.

@eli-schwartz
Copy link

If a user requests to opt in to BUILD_SHARED_LIBS, why are you ignoring their strongly-worded request to use shared libs?

Also what happens if you statically compile libshaderc.so against symbols from spirv-tools or whatever, and then try to use the same symbols in a thirdparty program that links to both spirv-tools and libshaderc?

Attempting to compile a shared library using static dependencies seems rather silly.

  • If you asked for non-default shared builds, then the logical assumption is you're trying to integrate into your system, for example, you're (a member of) a Linux distribution. (I'm a member of a linux distribution.) If https://github.com/google/shaderc is a project that doesn't integrate into your system, you are still going to have to delete the line

    add_subdirectory(third_party)
    and rm -rf third_party.

  • If you're trying to avoid depending on system libraries, you are going to use a static library for libshaderc too, because you don't want to worry about installing two files. You want it to Just Work, and that means statically compiling libshaderc into your application.

@dj2
Copy link
Contributor

dj2 commented Nov 15, 2018

You don't need to remove the third_party, you can override where shaderc gets third_party from so it will use the same spirv-tools as you. This is done in https://github.com/google/amber/blob/master/third_party/CMakeLists.txt#L179 to share the third_party files.

@eli-schwartz
Copy link

My spirv-tools are in /usr/lib, they are precompiled shared libraries, and there is no /usr/lib/CMakeLists.txt allowing me to add_subdirectory(/usr/lib).

@dj2
Copy link
Contributor

dj2 commented Nov 15, 2018

In which case, we may just want to add something like a USE_SYSTEM_LIBS flag to shaderc which avoids using its own third_party.

@haasn
Copy link

haasn commented Nov 15, 2018

In which case, we may just want to add something like a USE_SYSTEM_LIBS flag to shaderc which avoids using its own third_party.

That would be a good idea. Note that gentoo also opts to remove the third party entry. In general, no distro is going compile shaderc against its vendored dependencies. You could make many distro maintainers' livee easier by just supporting this directly.

Incidentally, that means more people will hit #463. So you might want to start taking that PR seriously if you plan on allowing users to build against non-bundled libs. (And as expected, gentoo also has to include this work-around)

@eli-schwartz
Copy link

While such an option would be useful, I still don't understand what the use case is for building a shared library and linking it against vendored static dependencies. It would make far more sense to have one option that toggled whether to build a fully static shaderc, or a fully shared shaderc.

The whole point of shared libraries is in order to share resources between many different projects. The shared library format is not much use if you don't, well, share it. I'm genuinely curious why someone afraid of relying on system copies of spirv-tools, glslang etc. would want to rely on system copies of shaderc either.

Seriously -- why does the entire computing world have such a hate-on for statically linking their applications? If your shared library is not tightly integrated into your operating system, it makes zero sense to use it at all. Static libraries are wonderful. They're the one and only, superior way to build programs that don't depend on fiddly external resources floating around outside your control.

Yes, this is me with my Linux distribution developer hat on.

@antiagainst
Copy link
Contributor Author

Hey @eli-schwartz:

Thanks a lot for your inputs on this!

Attempting to compile a shared library using static dependencies seems rather silly.

Yep, I'm not a Linux distribution developer, but my intent was trying to help. This pull request is not landed, so it's just one possible way of achieving the purpose. However, I'm quite happy to explain why I did it this way. :-)

The reason is that I thought Glsang & SPIRV-Tools didn't have their shared library story sorted out yet; they have not concretized on their versioning schemes, etc. (It may be untrue, or it may have already changed now, or maybe it does not matter; you can correct me on this.) If we just compile multiple .sos and let it be, it may introduce problems regarding compatibility of different components. (We've seen quite a few failures because Glslang & SPIRV-Tools out of sync, e.g., using different different versions of SPIRV-Headers.) So I was trying to hide all that complexity behind and just provide a single .so and wait until all the subprojects sort their .so stories out. You can always convert to multiple .sos later and this provides a pragmatic way to ship asap I think?

Besides, this is the way libshaderc_shared.so used to work, which I think is proven to work for some users (Chrome?).

As for the point of using shared library to share things, it's relative, right? Even with shared libraries, you still compile a bunch of symbols into the same .so, not every component gets their dedicated .so. So if more than one binary link to libshaderc.so, they still share the same libshaderc.so.

Hope this helps. :-)

@haasn
Copy link

haasn commented Nov 16, 2018

While this is sort of off-topic, the main point of using shared libraries, to me, has always been the ability to make updates to shared libraries (as long as they don't break ABI) without needing to touch all of the dependents.

Consider the use case of something like libjpeg.so. If there is a critical security vulnerability in libpng, all it requires to fix it is to drop in the new .so (with the same ABI) and restart the affected programs. In an entirely statically linked system, I also need to recompile everything on the system that was linked against libjpeg - and unlike with dynamic linking, there's no easy way to find out which programs this affects. Plus, I may not even have the source code for the components I am trying to relink. With a shared approach, I can still swap out what it links against in this instance. (Consider a video game using an old, buggy version of a dependency like libSDL2. With dynamic linking, I can swap out the lib for a fixed version and the game will work)

It's not just security vulnerabilities - if I want to change the behavior of a dependency, I can also easily do that in a way that affects the whole system. This is often the case for me as a developer. To give a silly but motivating example, I compile libjpeg.so with arithmetic decoding turned on. This means that everything which links against libjpeg.so dynamically is able to decode arithmetic JPEGs. But if a browser, say firefox, comes along and decides to use its own vendored libjpeg copy instead, it will not benefit from this change. Not a big deal in this instance but it can be annoying for more meaningful changes - especially if you're trying to test a patch that could potentially fix a problem you found in such a shared component. There's also something fundamentally dangerous about vendoring dependencies, because it promotes projects (again, firefox being an example), to apply their own patches to the vendored dependency without trying to submit them upstream. This is a very bad practice as it discourages merging back from upstream (due to the possibility of merge conflicts) and projects a clear disinterest in the rest of the free software community as a whole. If you have a fix or improvement to a dependency, you should rightfully submit it upstream so that everybody can benefit from it. Not doing so is not only harmful for the community as a whole, but also for the specific project.

Now, I realize that this reasoning does not apply that well to shaderc's vendored third party dependencies, because glslang & friends cannot be dynamically linked anyway. (They are only available as a weird collection of .a files). And that's probably a good thing, since I don't trust the glslang developers to bump their ABI version every time they break the ABI. (Heck, they don't even bump their version headers when they make breaking changes to the API). Actually, I don't really trust shaderc to do this either (and shaderc does not even have version headers, which makes it annoying to write forwards- and backwards-compatible code). But that's beside the point - the point is that even though glslang is linked against statically by shaderc, I still want my package manager to control which version of glslang is on my system. Imagine wanting to test out a new patch or a new version that could fix a problem in glslang. Right now, it's as simple as installing the new version of glslang and then re-installing shaderc. (The latter can even happen automatically). But if gentoo had not put in the effort to strip out the third_party from shaderc's build tools, I would first have to edit the shaderc build script to somehow point it at the new version of glslang.

Anyway, the bottom line is that (most) distributions want one component per package. No bundling three unrelated vendored dependencies inside the same package, that's simply not kosher. It makes things difficult to control for the user/maintainer, and unpredictable as to which version from which git repo is actually being used. In an ideal world, both glslang and shaderc would export dynamic libraries (the latter dynamically linking against the former), and each project would change the ABI version every time it breaks. But I realize that this is not really realistic, so we have to make compromises somewhere. And the proposed suggestion of adding a flag that allows you to build against system versions of the dependencies is IMHO a good compromise.

My 2 cents.

@eli-schwartz
Copy link

eli-schwartz commented Nov 16, 2018

@haasn,

Yes, you've just restated the argument in favor of tightly integrated distributions. :) But as I said,

If your shared library is not tightly integrated into your operating system, it makes zero sense to use it at all.

Linking to a private, vendored .so file means you don't get any of the benefits of shared libraries. And on top of that, you also don't get any of the benefits of static libraries (portability, reduced complexity in the ELF loader, smaller installation footprint). It's a lose-lose situation.

By definition, if you're using the static version of shaderc, you're not using the version controlled by your distribution... and thus, you are also not using the version of glslang controlled by your distribution.

@antiagainst,

The reason is that I thought Glsang & SPIRV-Tools didn't have their shared library story sorted out yet; they have not concretized on their versioning schemes, etc. (It may be untrue, or it may have already changed now, or maybe it does not matter; you can correct me on this.)

That sounds like a bug report for the (distribution?) vendor of the shaderc shared library. It would need to be rebuilt whenever updating glslang. Usually soname versions are used to detect this, but it's fully possible to rebuild it on every update too. And if it was statically linked, we'd need to rebuild it on every update anyway, because we would want to make sure it was using the latest version of glslang...

It doesn't matter, the current build process is completely broken anyway. If you make && make install the project as-is, it installs and overwrites the headers and static libraries for all of its third_party dependencies. This means that distributions need to apply custom patches to remove the third_party directory merely in order to create a version of shaderc that does not conflict with all those dependencies. And users who make install without the benefit of a package manager have it even worse, because they break their systems, and then the next time they update their system package for glslang, they overwrite the broken version with the fixed version, except oh no, now the headers don't match the statically built shaderc!

(This is, incidentally, why cmake as a build system sucks -- using add_subdirectory to include an entire thirdparty project is a broken model. See how meson handles this with subprojects)

tl;dr if you build with vendored third_party, you cannot and must not even attempt to install it, and your only sane option is to use a fully static libshaderc.a while at the same time manually configuring linker paths.

@haasn
Copy link

haasn commented Nov 16, 2018

Linking to a private, vendored .so file means you don't get any of the benefits of shared libraries.

There is one case that still benefits: Say a closed-source video game links against a vendored, old/buggy version of a library like libsdl2.so (or, indeed, libshaderc.so). As a result of this bug, the game fails to work on my system. Thanks to dynamic linking, I can still fix the bug in the .so and swap it out without needing access to the video game's source code. Doing the same with a statically linked, stripped binary, is basically impossible.

@antiagainst
Copy link
Contributor Author

Thanks for all the great and detailed explanation! I see the points now.

I'm not married to this implementation; happy to drop this pull request if you guys think it's really troublesome. :-)

@Kangz: WDYT since you did the original shared library support?

@Kangz
Copy link
Collaborator

Kangz commented Nov 16, 2018

No strong opinion, Dawn and Chrome and using the GN files now.

@antiagainst
Copy link
Contributor Author

Cool, I'll drop this pull request then. Thanks guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants