-
Notifications
You must be signed in to change notification settings - Fork 64
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
Optionally use the temp directory on windows instead of Ninja #51
Comments
+1. It feels pretty bad to require msys in the readme if you're using msvc on windows even with ninja and cmake on your path already. |
@mooman219 : Yes, we do not need to require msys2 for windows-msvc. I updated README.md with 53408c3. |
@Lokathor: So shaderc-sys is already quite complicated regarding build options right now. I'm a little bit reluctant to make it even more so. Is there a compelling reason that downloading ninja and putting it in path is not acceptable for you? I think using ninja is generally better as for compilation speed? But I am definitely not aware of your settings. I guess I'd like to know some context to understand it better. :) Besides, the temp directory prefix is not necessary shorter. It's |
Needing to install ninja is not hard but is one step further away from being able to do If there's alternatives that make it simpler to be able to make a window that says "hello world", I'll happily help implement and maintain them. |
(I say all of the following as a person who is myself the maintainer of a bindings crate, so I know somewhat the troubles you face) So, the temp directory seems to default to So say we're developing So naturally the files go into So since the limit is 240, then going from However, here's a better idea, I don't know why it didn't come to me before: Just take all the library files that we need to link against: And everyone else can just link to a pre-built lib file. and the whole crate will build in like 1 second instead of like 500 seconds. |
I still feel it's amusing that in the year of 2019, we are still fighting some historical limitation from DOS time... But anyway, seems things will be better with MSBuild 16 (or at least a good step forward I guess). @icefoxen: Yeah, it makes sense for me. Agreed that requiring an additional download step before building is not a pleasant thing for developers/users. People generally does not really look at README.md that much. Given shaderc-sys is likely a dependency of many other projects (which is nice ;-P), it helps to avoid the fraction. @Lokathor: thanks a lot for the detailed analysis! I see your point that moving to use Instead, what about this: I know that Visual Studio has already shipped CMake and Ninja: https://devblogs.microsoft.com/cppblog/cmake-support-in-visual-studio-whats-new-in-2017-15-3-update/. So I first check the path for Ninja. If not found, I then check the Visual Studio directory for it. The fallback can be using MSBuild and hope for the best. (SPIRV-Tools recently landed a commit to reduce the length, which hopefully will make MSBuild work again. I need to refresh the SPIRV-Tools hash in this repo to adopt it. But that means Python3 is forced then...) I'm also considering download Ninja. (Well downloading is a bit unconventional so we can renegotiate this, but we are already accessing the Internet using Git anyway.) WDYT? @Lokathor: Your idea of shipping pre-built binaries sounds interesting; I think we can explore that. |
I think that just having pre-built binary files on Win32 is the best option. It makes the build time for new builds drop down to 1-2 seconds. Unfortunately, as i recently learned when trying to add pre-built binary files to my own bindings crate, Linux and Mac aren't nearly as friendly to the idea, and those platforms might need to keep building shaderc from source. They don't have the path limit to worry about at least, though it still takes ages. |
Could it be set up so that instead of needing ninja the build script optionally uses the windows temp directory to build the
shaderc
lib, instead of the OUT_DIR path?The text was updated successfully, but these errors were encountered: