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

Windows Updates #2099

Closed

Conversation

joaocgreis
Copy link
Member

@joaocgreis joaocgreis commented Dec 18, 2019

This is all already in use by CI, but not yet by the release Jenkins. The release job need to be adjusted while landing this.

Notable changes:

  • VS2019 support
  • Windows now uses the VersionSelectorScript - it is a bit complex as expected, but it manages to filter for building node, building add-ons, and choose what runs where depending on what labels are available while always avoiding invalid combinations
  • Moved most scripts from CI jobs to the build repo - this allows us to reuse parts of the scripts for different jobs, gives us some history for long-term changes, and it is easy to change to a fork for testing if needed
  • Test building in debug mode
  • Test building the MSI
  • Python 3 added to all Windows machines
  • (EDIT) CMake added to all Windows machines
  • Windows moved to the main create worker playbook
  • Some documentation updates

Re-created all Windows machines to balance available versions optimizing for current master.

Refs: #1996

@@ -34,6 +37,9 @@ Supported versions for building Node.js from source.
| v9 | 2015, VCBT2015, 2017 |
| v10 | 2017 <sup>[5]</sup> |
| v11 | 2017 |
| v12 | 2017, 2019 (flag) <sup>[8]</sup> |
| v13 | 2017, 2019 <sup>[9]</sup> |
| v13 | 2017, 2019 |
Copy link
Member

Choose a reason for hiding this comment

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

Is this line supposed to be v14?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, fixed, thanks!

@joaocgreis
Copy link
Member Author

Added a commit to install CMake (#2097) and rebased.

@richardlau
Copy link
Member

Added a commit to install CMake (#2097) and rebased.

It's not part of the Native Desktop Workload for Visual Studio? We have https://ci.nodejs.org/job/libuv-test-commit-windows-cmake/ already running in the CI.

@joaocgreis
Copy link
Member Author

As I understand it, using CMake from VS is a bit of an hack. Needs to be accessed by full path, so location will change with VS version. Also, it's not present in VS2015. Having it from Chocolatey should be a better option, more so if we start using it to build Node at some point.


# Retry once
git archive --format=tar --remote="$TEMP_REPO" $TEMP_BRANCH src/node_version.h -o node_version.h.tar ||
git archive --format=tar --remote="$TEMP_REPO" $TEMP_BRANCH src/node_version.h -o node_version.h.tar
Copy link
Member

Choose a reason for hiding this comment

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

this is interesting, fetching a single file from a remote repo? I get protocol problems trying this out locally so can't seem to replicate this behaviour.

Copy link
Member Author

Choose a reason for hiding this comment

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

I also had issues with http repos, but this works for our temp repo (ssh). This is already being used by node-test-commit-windows-fanned (running in jenkins-workspace).

[ /vs2019-x86$/, testType, lt(14) ],
// VS versions supported to build add-ons
[ /vs2013-COMPILED_BY/, testType, gte(9) ],
[ /vs2015-COMPILED_BY/, testType, noVer ],
Copy link
Member

Choose a reason for hiding this comment

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

👍 good idea to include these with noVer for completeness and documentation purposes, I like

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

hard to grok too much of this without better knowledge of windows & ansible+windows and a bunch of other stuff in here so this is mostly a rubber-stamp. Some comments inline, nothing major blocking, except maybe the pip2 vs pip3.

Copy link
Contributor

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

Very impressive work! Thanks much!

# TODO: Ensure no other versions are installed
- name: install Visual Studio 2013
include_tasks: "partials/vs2013.yml"
- name: install Visual Studio 2013
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

VS2013 is used to test add-ons for Node.js v8. We should keep the machines available for some time after a Node version goes EOL, but anyway I'd like this to be in the commit history for documentation (we might need to test an old issue or something).

ansible/roles/visual-studio/tasks/partials/vs2019.yml Outdated Show resolved Hide resolved
@joaocgreis
Copy link
Member Author

Thanks for the reviews! This needs some work to land, iojs+release needs to have the same changes as the test release job, and the jobs in CI need to be changed to point at master. I'll do it later this week.

joaocgreis added a commit that referenced this pull request Jan 2, 2020
PR-URL: #2099
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
joaocgreis added a commit that referenced this pull request Jan 2, 2020
PR-URL: #2099
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
joaocgreis added a commit that referenced this pull request Jan 2, 2020
Following Azure security recommendations.

PR-URL: #2099
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
joaocgreis added a commit that referenced this pull request Jan 2, 2020
PR-URL: #2099
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
joaocgreis added a commit that referenced this pull request Jan 2, 2020
PR-URL: #2099
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
joaocgreis added a commit that referenced this pull request Jan 2, 2020
Specific commit and floating PRs known to work.

PR-URL: #2099
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
joaocgreis added a commit that referenced this pull request Jan 2, 2020
PR-URL: #2099
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
joaocgreis added a commit that referenced this pull request Jan 2, 2020
Refs: #2097
PR-URL: #2099
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
joaocgreis added a commit that referenced this pull request Jan 2, 2020
PR-URL: #2099
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
joaocgreis added a commit that referenced this pull request Jan 2, 2020
PR-URL: #2099
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
joaocgreis added a commit that referenced this pull request Jan 2, 2020
PR-URL: #2099
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
joaocgreis added a commit that referenced this pull request Jan 2, 2020
PR-URL: #2099
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
joaocgreis added a commit that referenced this pull request Jan 2, 2020
PR-URL: #2099
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
joaocgreis added a commit that referenced this pull request Jan 2, 2020
PR-URL: #2099
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
joaocgreis added a commit that referenced this pull request Jan 2, 2020
PR-URL: #2099
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Christian Clauss <cclauss@me.com>
@joaocgreis
Copy link
Member Author

Thanks!

Adapted the jobs on both Jenkins instances.

Landed in 1dd97fa...36202a2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants