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

win, build: generate .sln only when necessary #21284

Closed
wants to merge 6 commits into from

Conversation

bzoz
Copy link
Contributor

@bzoz bzoz commented Jun 12, 2018

When generating project files, store flags passed to configure. Next time, if node.sln exists and configure flags match those stored, skip building .sln files.

Adds forceprojgen vcbuild option to force project files regeneration.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

When generating sln, store flags passed to configure. Next time, if
node.sln exists and configure flags match those stored, skip building
.sln files.

Adds forceprojgen vcbuild option to force .sln regeneration.
@nodejs-github-bot
Copy link
Collaborator

@bzoz sadly an error occured when I tried to trigger a build :(

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. windows Issues and PRs related to the Windows platform. labels Jun 12, 2018
@bzoz
Copy link
Contributor Author

bzoz commented Jun 13, 2018

vcbuild.bat Outdated
@@ -67,6 +68,7 @@ if /i "%1"=="x86" set target_arch=x86&goto arg-ok
if /i "%1"=="x64" set target_arch=x64&goto arg-ok
if /i "%1"=="vs2017" set target_env=vs2017&goto arg-ok
if /i "%1"=="noprojgen" set noprojgen=1&goto arg-ok
if /i "%1"=="forceprojgen" set forceprojgen=1&goto arg-ok
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe projgen? so it goes as a pair projgen/noprojgen

vcbuild.bat Outdated
@@ -252,10 +254,24 @@ goto build-doc

:project-gen
@rem Skip project generation if requested.
if defined noprojgen goto msbuild
SETLOCAL EnableDelayedExpansion
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is only needed for L262 & L263, could this be moved there? Otherwise I can see this giving someone a headache in the future.

SETLOCAL EnableDelayedExpansion
set /p prev_configure_flags=<used_configure_flags
if NOT !prev_configure_flags! == !configure_flags! ENDLOCAL & goto run-configure

vcbuild.bat Outdated
@rem Generate the VS project.
echo configure %configure_flags%
echo %configure_flags%> used_configure_flags
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we rename the tmp file .used_configure_flags. I know this is not *nix, but git will exclude it by default, and Windows will at least group it together with the rest of the settings/temporary files.

@refack refack added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 13, 2018
@refack
Copy link
Contributor

refack commented Jun 13, 2018

Great idea!
I'm not sure of the semver-ity since this will change the stdout of vcbuild.bat. IMHO It's semver minor, unless someone can indicate otherwise, or knows for sure of some downstream project that uses the output of python configure or vcbuild.bat?

/CC @nodejs/build-files @nodejs/platform-windows @zcbenz

@seishun
Copy link
Contributor

seishun commented Jun 13, 2018

What's the cost of always regenerating .sln files?

@refack
Copy link
Contributor

refack commented Jun 13, 2018

What's the cost of always regenerating .sln files?

From my POV:

  1. It overwrites manual edits
  2. Changing the "create time" of the .sln & .vcxproj files forces MSBUILD to recompile
  3. There's the "noise" from the configure run
  4. Time

@refack refack removed the meta Issues and PRs related to the general management of the project. label Jun 13, 2018
@bzoz
Copy link
Contributor Author

bzoz commented Jun 13, 2018

@refack: updated with your comments, PTAL

Also, project files will always be generated for build-release builds.

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@refack
Copy link
Contributor

refack commented Jun 13, 2018

I'm thinking as a follow up PR to move this logic to configure, based on mtime of the .gyp? files.

@richardlau
Copy link
Member

I'm thinking as a follow up PR to move this logic to configure, based on mtime of the .gyp? files.

I'd much prefer this. With this PR as it stands if you tweak a .gyp file you will need to force project files generation, which isn't documented anywhere.

@refack
Copy link
Contributor

refack commented Jun 13, 2018

I'd much prefer this.

Time....

Also

tweak a .gyp file

Don't!
I'm obviously kidding

@refack
Copy link
Contributor

refack commented Jun 13, 2018

I'm thinking as a follow up PR to move this logic to configure, based on mtime of the .gyp? files.

I'd much prefer this.

A very rough sketch:

diff --git a/vcbuild.bat b/vcbuild.bat
index 717950bee9..a9ff3a423a 100644
--- a/vcbuild.bat
+++ b/vcbuild.bat
@@ -258,20 +258,22 @@ goto build-doc
 if defined noprojgen goto skip-configure
 if defined projgen goto run-configure
 if not exist node.sln goto run-configure
-if not exist .used_configure_flags goto run-configure
-SETLOCAL EnableDelayedExpansion
-set /p prev_configure_flags=<.used_configure_flags
-if NOT !prev_configure_flags! == !configure_flags! ENDLOCAL && goto run-configure
-ENDLOCAL
+if not exist .gyp_configure_stamp goto run-configure
+where /R . /T *.gyp? > .tmp_gyp_configure
+echo %configure_flags% >> .tmp_gyp_configure
+fc .gyp_configure_stamp .tmp_gyp_configure > NUL
+if ERRORLEVEL 1 goto run-configure

 :skip-configure
-echo Reusing solution generated with %prev_configure_flags%
+del .tmp_gyp_configure
+echo Reusing solution generated with '%prev_configure_flags%'
 goto msbuild

 :run-configure
 @rem Generate the VS project.
 echo configure %configure_flags%
-echo %configure_flags%> .used_configure_flags
+where /R . /T *.gyp? > .gyp_configure_stamp
+echo %configure_flags% >> .gyp_configure_stamp
 python configure %configure_flags%
 if errorlevel 1 goto create-msvs-files-failed
 if not exist node.sln goto create-msvs-files-failed

Unfortunately I can't test this ATM

@bzoz
Copy link
Contributor Author

bzoz commented Jun 14, 2018

Added a warning message that is displayed after build fails with reused solution files:

Building Node with reused solution failed. To regenerate project files use "vcbuild projgen"

@addaleax
Copy link
Member

@BridgeAR
Copy link
Member

Is this ready to land?

@bzoz
Copy link
Contributor Author

bzoz commented Jun 20, 2018

@BridgeAR: I would like to try out @refack idea (since it sounds very good) before landing this

@bzoz
Copy link
Contributor Author

bzoz commented Jun 21, 2018

@refack idea worked out great, added it to the PR. PTAL.

@bzoz
Copy link
Contributor Author

bzoz commented Jun 26, 2018

@bzoz
Copy link
Contributor Author

bzoz commented Jun 26, 2018

landed in 4dece04

@bzoz bzoz closed this Jun 26, 2018
bzoz added a commit that referenced this pull request Jun 26, 2018
When generating sln, store flags passed to configure. Next time, if
node.sln exists and configure flags match those stored, skip building
.sln files.

Adds projgen vcbuild option to force .sln regeneration.

PR-URL: #21284
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jun 28, 2018
When generating sln, store flags passed to configure. Next time, if
node.sln exists and configure flags match those stored, skip building
.sln files.

Adds projgen vcbuild option to force .sln regeneration.

PR-URL: #21284
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Jul 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. semver-minor PRs that contain new features and should be released in the next minor version. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants