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

build,win: put all compilation artifacts into out #27149

Merged
merged 1 commit into from
Apr 12, 2019

Conversation

refack
Copy link
Contributor

@refack refack commented Apr 9, 2019

Solves (almost completely) the unnecessary distribution of build artifacts all over the repo.
In order to keep this semver-patch I've added a symlinking step:
https://github.com/nodejs/node/blob/2ee659cde1975a247dfa3d30d765eb9fdd3973f4/vcbuild.bat#L326-L327

Also has some small tweaks.

what's left to solve is the locations of the .vcxproj files. That requires a GYP patch

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

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

@nodejs-github-bot
Copy link
Collaborator

@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 Apr 9, 2019
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Not a Windows expert but the changes LGTM.

node.gyp Outdated Show resolved Hide resolved
@refack refack self-assigned this Apr 9, 2019
@refack refack added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 9, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs nodejs deleted a comment from nodejs-github-bot Apr 12, 2019
* Add symlink from Release to out\Release for backward compat

PR-URL: nodejs#27149
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@refack refack added the notable-change PRs with changes that should be highlighted in changelogs. label Apr 12, 2019
@refack refack merged commit 25df3c1 into nodejs:master Apr 12, 2019
@refack refack deleted the gyp-opt-fix-win-out-dir branch April 12, 2019 19:22
@richardlau
Copy link
Member

@refack:

I've just spent a long time trying to work out why changes I made weren't being reflected when I run node... It turns out on Windows 7 I'm getting this:

  node.vcxproj -> out\Release\\node.exe
You do not have sufficient privilege to perform this operation.

C:\work\node\github\nodejs>

i.e.

if EXIST out\%config% mklink /D %config% out\%config%
is failing.

Unfortunately I didn't immediately spot this and ended up running the old Release\node from a previous compilation before this PR landed (tools/test.py still looks for Release\node). It works if I run the command prompt as administrator but I previously did not have to do that to build Node.js.

Does this work on later versions of Windows without Administrator privileges? (I can't check on Windows 10 until Monday at the earliest).

@refack
Copy link
Contributor Author

refack commented Apr 13, 2019

Thanks for the report.

  node.vcxproj -> out\Release\\node.exe
You do not have sufficient privilege to perform this operation.

C:\work\node\github\nodejs>

Did vcbuild.bat exit at that point?

Does this work on later versions of Windows without Administrator privileges? (I can't check on Windows 10 until Monday at the earliest).

It was an issue that was addressed by MS for Windows 10 (at least, maybe even for Windows 8.1).
I run my dev env in a non privileged cmd session, so this should be Ok. I'll triple check.

On CI we do run under an admin account by in an non privileged session, so that's half way to a proper testing env (we do not CI test with Windows 7, but we do with Server2008R2 which is the equivalent Server release).

(tools/test.py still looks for Release\node).

Originally I fixed that, but choose to revert because of CI legacy issues...

P.S. posted a notice at https://github.com/orgs/nodejs/teams/collaborators/discussions/72

@richardlau
Copy link
Member

Thanks for the report.

  node.vcxproj -> out\Release\\node.exe
You do not have sufficient privilege to perform this operation.

C:\work\node\github\nodejs>

Did vcbuild.bat exit at that point?

No it continues, which is why I missed it the first few times and only really spotted it when I cut down my vcbuild.bat invocation to vcbuild.bat openssl-no-asm.

Does this work on later versions of Windows without Administrator privileges? (I can't check on Windows 10 until Monday at the earliest).

It was an issue that was addressed by MS for Windows 10 (at least, maybe even for Windows 8.1).
I run my dev env in a non privileged cmd session, so this should be Ok. I'll triple check.

On CI we do run under an admin account by in an non privileged session, so that's half way to a proper testing env (we do not CI test with Windows 7, but we do with Server2008R2 which is the equivalent Server release).

(tools/test.py still looks for Release\node).

Originally I fixed that, but choose to revert because of CI legacy issues...

ack

Trott pushed a commit to Trott/io.js that referenced this pull request May 2, 2019
Seems at least some windows versions interpret symlinks not as folders
therefore .gitignore needs some extra entries.

PR-URL: nodejs#27484
Refs: nodejs#27149
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
targos pushed a commit that referenced this pull request May 4, 2019
Seems at least some windows versions interpret symlinks not as folders
therefore .gitignore needs some extra entries.

PR-URL: #27484
Refs: #27149
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@joyeecheung
Copy link
Member

joyeecheung commented May 11, 2019

I think the logic added here is off. I got this from a fresh clone:

C:\Users\joyee\projects\node>rd Release
The system cannot find the file specified.

C:\Users\joyee\projects\node>if errorlevel 1 echo "Old build output exists at 'out\Release'. Please remove."   & exit /B

C:\Users\joyee\projects\node>if EXIST out\Release mklink /D Release out\Release
You do not have sufficient privilege to perform this operation.

C:\Users\joyee\projects\node>if errorlevel 1 exit /B

I currently just comments out this logic as a workaround (cctest does not work if I do that, it would be better if we just replace %config% with out/%config% in the script), but there might be other people running into this.

@joyeecheung
Copy link
Member

joyeecheung commented May 11, 2019

I fixed the permission issue by turning on Developer mode which allows mklink without administrator privileges (there is probably some group policy for this but I got lazy). A note about this should probably be PR'd into BUILDING.md...

@richardlau
Copy link
Member

I think the logic added here is off. I got this from a fresh clone:

C:\Users\joyee\projects\node>rd Release
The system cannot find the file specified.

C:\Users\joyee\projects\node>if errorlevel 1 echo "Old build output exists at 'out\Release'. Please remove."   & exit /B

C:\Users\joyee\projects\node>if EXIST out\Release mklink /D Release out\Release
You do not have sufficient privilege to perform this operation.

C:\Users\joyee\projects\node>if errorlevel 1 exit /B

I currently just comments out this logic as a workaround (cctest does not work if I do that, it would be better if we just replace %config% with out/%config% in the script), but there might be other people running into this.

I'm currently setting up a new Windows 10 install and am also running into this. I'll try turning on developer mode.

@refack
Copy link
Contributor Author

refack commented May 16, 2019

I'm currently setting up a new Windows 10 install and am also running into this. I'll try turning on developer mode.

Could you check if mklink /J Release out\Release works without elevation or dev-mode?

@richardlau
Copy link
Member

I'm currently setting up a new Windows 10 install and am also running into this. I'll try turning on developer mode.

Could you check if mklink /J Release out\Release works without elevation or dev-mode?

C:\work\nodejs\github\node>mklink /D Release out\Release
You do not have sufficient privilege to perform this operation.

C:\work\nodejs\github\node>mklink /J Release out\Release
Junction created for Release <<===>> out\Release

C:\work\nodejs\github\node>

@refack
Copy link
Contributor Author

refack commented May 16, 2019

C:\work\nodejs\github\node>mklink /J Release out\Release
Junction created for Release <<===>> out\Release

Thanks!
Opened #27736

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. notable-change PRs with changes that should be highlighted in changelogs. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants