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.

Sign up for GitHub

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

Jump to bottom

build: conditionally clear vcinstalldir #36009

Merged
merged 0 commits into from
Nov 13, 2020
Merged

build: conditionally clear vcinstalldir #36009

merged 0 commits into from
Nov 13, 2020

Conversation

bingenito
Copy link
Contributor

For scenario where target env is explicitly specified as vs2019, do not clear VCINSTALLDIR which was being cleared to handle fallback to vs2017 block when attempting to find a matching available VS.

Fixes: #35856

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

THIS SOFTWARE IS CONTRIBUTED SUBJECT TO THE TERMS OF . https://github.com/nodejs/node/blob/master/LICENSE.
THIS SOFTWARE IS LICENSED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE AND ANY WARRANTY OF NON-INFRINGEMENT, ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. THIS SOFTWARE MAY BE REDISTRIBUTED TO OTHERS ONLY BY EFFECTIVELY USING THIS OR ANOTHER EQUIVALENT DISCLAIMER IN ADDITION TO ANY OTHER REQUIRED LICENSE TERMS.

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform. labels Nov 6, 2020
@Trott
Copy link
Member

Trott commented Nov 9, 2020

@nodejs/platform-windows

@bingenito
Copy link
Contributor Author

I realized my commit message format was wrong so amended and pushed

@Trott
Copy link
Member

Trott commented Nov 10, 2020

@nodejs/build-files

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 12, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 12, 2020
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/34369/

@bingenito
Copy link
Contributor Author

@Trott If this does get merged, is there any guidance or steps I should traverse to get it merged back to 14?

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/34378/

@Trott
Copy link
Member

Trott commented Nov 12, 2020

@Trott If this does get merged, is there any guidance or steps I should traverse to get it merged back to 14?

Should happen by default, I think. The hard part is going to be to find Windows folks who can review it and approve it. /ping @joaocgreis @bzoz

richardlau
richardlau approved these changes Nov 12, 2020
View reviewed changes
Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

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

As I mentioned in #35856 (comment) I'm unable to validate #30119 still works but it looks like it should.

@richardlau
Copy link
Member

@Trott If this does get merged, is there any guidance or steps I should traverse to get it merged back to 14?

Should happen by default, I think. The hard part is going to be to find Windows folks who can review it and approve it. /ping @joaocgreis @bzoz

I'll stick a watch label on it for 14.x anyway. Would still be good to get someone from @nodejs/platform-windows to review.

@bingenito
Copy link
Contributor Author

As I mentioned in #35856 (comment) I'm unable to validate #30119 still works but it looks like it should.

@richardlau I should be able to dig up a pc with vs2017 tomorrow and include log snippet to help with evidence to ensure it falls through.

@richardlau
Copy link
Member

As I mentioned in #35856 (comment) I'm unable to validate #30119 still works but it looks like it should.

@richardlau I should be able to dig up a pc with vs2017 tomorrow and include log snippet to help with evidence to ensure it falls through.

Just remembered that we dropped VS2017 support in Node.js 15 (#33694) so there's nothing to fall through to on master/v15.x. Node.js 14 does still support VS2017 so we should try to ensure the fallback still works there.

@bingenito
Copy link
Contributor Author

@richardlau @Trott
As current is a non-issue given vs2017 support removed, I switched specifically to v14.x to run these tests under a Developer Command Prompt for VS 2017 on a pc with no vs2019

Without change:

C:\temp\node>vcbuild
Looking for Python
Python found in \\<redacted>\python.exe
Looking for NASM
Looking for Visual Studio 2019
Looking for Visual Studio 2017
Found MSVS version 15.0
Reusing solution generated with  --dest-cpu=x64
  histogram.vcxproj -> ..\..\out\Release\lib\histogram.lib

With change:

C:\temp\node>vcbuild
Looking for Python
Python found in \\<redacted>\python.exe
Looking for NASM
Looking for Visual Studio 2019
Looking for Visual Studio 2017
Found MSVS version 15.0
Reusing solution generated with  --dest-cpu=x64
  histogram.vcxproj -> ..\..\out\Release\lib\histogram.lib

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/34382/

@Trott Trott closed this Nov 13, 2020
@Trott
Copy link
Member

Trott commented Nov 13, 2020

Landed in 4b0308a.

Thanks for the contribution! 🎉

@Trott Trott merged commit 4b0308a into nodejs:master Nov 13, 2020
@bingenito bingenito deleted the ISSUE-35856 branch November 13, 2020 16:54
codebytere pushed a commit that referenced this pull request Nov 22, 2020
@bingenito @codebytere
build: conditionally clear vcinstalldir
fbe210b 
For scenario where target env is explicitly specified as vs2019, do
not clear VCINSTALLDIR which was being cleared to handle fallback to
vs2017 block when attempting to find a matching available VS.

Fixes: #35856

PR-URL: #36009
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Nov 22, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Dec 9, 2020
@bingenito @BethGriggs
build: conditionally clear vcinstalldir
10440fd 
For scenario where target env is explicitly specified as vs2019, do
not clear VCINSTALLDIR which was being cleared to handle fallback to
vs2017 block when attempting to find a matching available VS.

Fixes: #35856

PR-URL: #36009
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
@bingenito @BethGriggs
build: conditionally clear vcinstalldir
e749d4b 
For scenario where target env is explicitly specified as vs2019, do
not clear VCINSTALLDIR which was being cleared to handle fallback to
vs2017 block when attempting to find a matching available VS.

Fixes: #35856

PR-URL: #36009
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
@bingenito @BethGriggs
build: conditionally clear vcinstalldir
6bafe04 
For scenario where target env is explicitly specified as vs2019, do
not clear VCINSTALLDIR which was being cleared to handle fallback to
vs2017 block when attempting to find a matching available VS.

Fixes: #35856

PR-URL: #36009
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau approved these changes

@Trott Trott Trott approved these changes

@joaocgreis joaocgreis Awaiting requested review from joaocgreis

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

vcbuild.bat fails for Visual Studio 2019 if VCINSTALLDIR intentionally pre-defined
5 participants
@bingenito @Trott @nodejs-github-bot @richardlau @BethGriggs