-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
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
Update VS project generation #11742
Update VS project generation #11742
Conversation
Aren't the parallel jobs broken with msvc? Something to do with race conditions. |
@Zephilinox I think it was, but now I'm building it on 4 cores (-j4) and it works flawlessly. Maybe it was the problem with older SCons? Also without my PR it generates project with -j2, so it is broken anyway if it really is. |
I tested it out on my machine and for me it didn't really work using Scons 3.0.0. When trying to build from Visual Studio I got a "scons: *** No SConstruct file found" error. Then I downgraded to Scons 2.5.1 as per the issue thread's recommendation and tried this branch again. This time the build actually started, but I'm getting some weird errors like "pow: ambiguous call to overloaded function" and "cast to type String () is not allowed". Will keep testing. |
@vexille Thank you, it's really weird. You ran scons p=windows vsproj=yes first, right? |
@Listwon yes. Actually running scons with multiple jobs on the command line before generating the vsproject always results in some error, but just a clean Just to make sure I'm not misleading anything though, this whole thing on godot's master branch doesn't work for me either, multithreaded or not. And actually I went back to the issue page and the SCons version mentioned is the 2.4.1. I'll try that out in the master branch and in your branch and report back. |
@vexille AFAIK unfortunately the first run must be single threaded. I wonder if there is any workaround to apply in SConstruct. I'm also trying to fix the intellisense now (I know what and why is broken, but the problem is how to change it and not break the build ;) ). |
@Listwon well it's not so bad if all the rest works fine, about the first single threaded run. The intellisense thing for me is solved by adding the directories by hand in the includes section for the project, is that the direction you're going? |
@vexille Yeah, exactly! The problem is that it autogenerates directories from CPPPATH, which contains SCons specific '#' symbol. It should be ommited or changed to $(ProjectDir)\, but it's not that easy. |
@vexille I did it! I've just fixed the Intellisense 😍 Please check this. |
Yep, it works, intellisense is working fine! As for the actual build though, no such luck. But I'm getting the same problem on master, so it's nothing to do with your changes. Sorry I can't help you more with that :( By the way, for what it's worth, I managed to do the first run of the scons call with -j6 without any problems. But I did downgrade to Scons 2.4.1, so maybe that's the magic version :P |
@vexille Thanks for your help :) I based my changes on master. I wonder what causes the problem. I'm so glad that Intellisense is working as intended now :) |
For the record, I finally got everything to build! I had to change a couple of lines to add a float cast:
But actually those files are pretty different in master, so I guess they got changed quite a bit in branch 2.1 already. Anyway, I'll do a clean checkout of 2.1 and test it out, if I can solve the problem the same way, I'll open a PR of my own, I guess. |
Tested with SCons 2.5.1 - works. The caveat is it detects VS 2015 batch file (to append in build paths for NMake inside vsproject), not VS 2017. For VS 2017 toolchain SCons 3.0 is necessary. |
@vexille
I had this happen too. SCons 3.0.0 VS 2017 -> Project Properties -> NMake -> Build Command Line : This build.bat script wasn't working :
So. SCons was starting, but not seeing the SConstruct file. Ran So amending my build.bat script
Then VS 2017 could build Godot fine. How it works : Note : vcvars32.bat and vcvars64.bat are wrappers |
@Grass-H0PEr Yeah, this is one of the possible solutions. In this PR I fixed it by passing source directory as an argument to scons: |
SConstruct
Outdated
@@ -147,6 +154,7 @@ opts.Add('vsproj', "Generate Visual Studio Project. (yes/no)", 'no') | |||
opts.Add('warnings', "Set the level of warnings emitted during compilation (extra/all/moderate/no)", 'no') | |||
opts.Add('progress', "Show a progress indicator during build (yes/no)", 'yes') | |||
opts.Add('dev', "If yes, alias for verbose=yes warnings=all", 'no') | |||
opts.Add('num_jobs', "Number of parallel builds", '2') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a misleading option, as it's used only for vsproj. What it advertises is what the actual -j
option does. Something like vsproj_jobs
included below vsproj
would be clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Roger, I will fix it.
SConstruct
Outdated
if (env.msvc): | ||
env.Append(CCFLAGS=['/D' + header[1]]) | ||
else: | ||
env.Append(CCFLAGS=['-D' + header[1]]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds like overkill and will make the command line length explode no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akien-mga It was taken directly from master branch and it works. Maybe it needs optimisation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akien-mga Sorry, it's not even usable on 2.1 branch. On master branch there is one SCsub with that. I removed this chunk.
Update VS project generation and add num_jobs option (default value: 2 - the same as before) for parallel builds inside Visual Studio. Tested with SCons 3.0 and Visual Studio 2017.
Fix the CPPPATH passed to MSVSProject. Uses SCons Dir() function to convert "#" paths http://www.scons.org/doc/0.97/HTML/scons-user/a3414.html#CV-CPPPATH
- Cherry picked godotengine#10662 and fixed merge conflicts. - Manualy merged the change from godotengine#11904. - Did not merge godotengine#12236 since I'm not sure whether the issue affects Godot 2.1 and I don't have VS2013 to test. - Did not merge godotengine#11843 since it doesn't seem relevant (the code is only needed for creating DONORS.md, etc.). - Did not merge godotengine#10727 and godotengine#11752 since they seem to be already included in godotengine#11742. - The Windows and Linux builds have been tested with Scons 3.0 using Python 3. - OSX and iOS should hopefully work but are not tested since I don't have a Mac. - Builds using SCons 2.5 and Python 2 should not be impacted.
Update VS project generation and add num_jobs option (default value: 2 - the same as before) for parallel builds inside Visual Studio. Tested with SCons 3.0 and Visual Studio 2017.
[Edit]
SCons 3.0 is necessary to create proper vsproject for Visual Studio 2017 toolchain (earlier SCons version doesn't detect VS 2017 batch file).