Improved shaderc-sys Build Structure and Fixes
#163
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Move shaderc search path resolution into a function * Replace `if path.none()` checks with early returns * Replace unnecessary `if` with `and_then`
* Moved build from source code into a function * Build from source if requested before path resolution * Changed `build_shaderc_unix` to take a `&str` for OS
* Removed additional check for `msvc` env when deciding lib name * Use verbatim modifier for windows link libs to prevent rustc from changing the name
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This reverts commit 6f915f6.
|
Reverted the changes to path resolution for now. If I can somehow get something definitively working I'll make another PR just for that. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NOTE: changes are actually quite a bit less expansive than they look; most of the diff is just from rearranging things
Overview
Cleans up search path resolution for
shadercby extracting it into a function (get_search_dir) and replacing the endless cycle ofif path.is_none() { path = ... }checks with early returns.Fixes an issue where path resolution did not line up with the docs; it is documented that the
build-from-sourcefeature takes precedence over all other resolution methods, but in reality it only disabled a few last-resort checks and changed a single warning message. Now, this feature actually overrides any attempt at path resolution immediately. Since building from source is still attempted if resolution fails, the associated code has also been moved into a separate function (build_from_source).REVERTED (see comments)
Finally, the check for MSVC environments when deciding the library name to search for has been removed, and therustc-link-liboptions for non-source-build windows environments now specifically searches for<LIB_NAME>.libusing the:+verbatimmodifier to prevent rustc (or the C++ compiler) from changing it. This fixes an issues where windows systems with the Vulkan SDK installed would fail to find theshaderclibrary during builds (in non-MSVC environments), always forcing a build from source.Breaking Changes
The
build-from-sourcefeature now works as documented. Anyone relying on the old (broken) behavior could see breakage, though this is probably unlikely.REVERTED (see comments)
On windows, the expected library name during path resolution is nowSHADERC_STATIC_LIB_FILE_WIN(shaderc_combined.lib), as opposed toSHADERC_STATIC_LIB_FILE_UNIX(libshaderc_combined.a) in non MSVC-environments.This does not affect source builds.
Discussion
REVERTED (see comments)
As for the changes to path resolution on windows, I personally think the method here is an improvement over the previous design. At least from the names, the library name constants are clearly meant to be OS-specific; the host compiler should have zero bearing on this (if they are meant to be env-specific instead, they should explicitly name themselves as such). Most noticeably, it certainly has no bearing on the structure of the host Vulkan installation (if any); this is what preventedshaderc-rsfrom finding the existing libraries before. Note that both clang and MinGW support linking to.liblibraries on windows (or at least theshaderc_combined.libshipped with Vulkan), though granted I'm not sure MinGW actually has a compatible ABI since I'm assuming the shipped libraries were built with MSVC? It's possible this won't work in practice for MinGW at least, but it should for Clang. Maybe MinGW environments should skip the Vulkan check entirely with a warning?Still, this could be annoying in some cases. For example it means users cannot always replicate the source-build procedure this project uses themselves (usingSHADERC_LIB_DIR) since (at least for me, using MinGW) the underlyingshaderclibrary produces UNIX-style libraries.If this is a concern, I could easily check for both names on non-MSVC systems instead, which would make at least that change back-compatible.