Add windows support to github actions (new branch) #144
Conversation
| # windows build agents don't support the path lengths required for initializing arcore dependencies, | ||
| # so we manually initialize the submodules we need here. |
There was a problem hiding this comment.
Ok, I think this answers my earlier question about the submodule update. Instead of having an inclusion list like this, could we have an exclusion list? That might make it easier to maintain. Seems like this is possible? https://stackoverflow.com/a/52185169/7318670
There was a problem hiding this comment.
updated to match suggestion
| @@ -1,5 +1,18 @@ | |||
| Import-Module $PSScriptRoot\Utils.psm1 | |||
There was a problem hiding this comment.
What is the advantage of having these separate PowerShell scripts as opposed to just having all this logic in gulpfile.js?
There was a problem hiding this comment.
The only thing here currently that benefits from powershell scripts is the logic for resolving msbuild. For local developer setups where msbuild isn't added to the path variable, we download the vs installer powershell module which helps us resolve the directory to look in for msbuild. I'm not sure if something similar exists with gulp.
There was a problem hiding this comment.
I am going to keep some of the helper logic in the Utils.psm1 script, but i have reduced powershell usage. Setup is now conducted through gulp tasks. Only build logic is facilitated through powershell now.
|
|
||
| $MSBuild = Get-MSBuildPath | ||
| & "$MSBuild" /p:Configuration="$Configuration" /p:Platform="$Platform" $Solution | ||
| & "$MSBuild" /p:Configuration="$Configuration" /p:Platform="$Platform" /m:32 $Solution |
There was a problem hiding this comment.
Should this just be /m? My understanding is that that tells MSBuild to build in parallel, but lets it decide how much parallelism based on the number of cpu cores in the machine where it is actually building.
There was a problem hiding this comment.
Also, within a single project, the vc++ compiler can compile c++ files in parallel with the /MP compiler switch. Can/do/should we use that as well?
There was a problem hiding this comment.
specifying processors was more confusion on my end. If msbuild can auto determine the correct number of processors, I would prefer that as well.
/MP is already set through cmake for majority if not all BabylonNative static libs. It is also set for BabylonReactNative.dll
There was a problem hiding this comment.
removed the number of processors from m
There was a problem hiding this comment.
FYI, you might want to consider enabling parallelism on the project level too. Looks like there's been some improvements added in Visual Studio 2019 16.3 - https://devblogs.microsoft.com/cppblog/improved-parallelism-in-msbuild/
There was a problem hiding this comment.
Parallelism feels beyond the scope of this review. I've filed an issue here to track: BabylonJS/BabylonNative#584
3 participants
This review contains the following changes: