Skip to content

jenkins,doc: update supported Visual Studio for v21 #3484

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 2 commits into from
Oct 7, 2023

Conversation

targos
Copy link
Member

@targos targos commented Sep 13, 2023

Refs: nodejs/node#49051

/cc @StefanStojanovic I think the test CI is ready for this, but the release CI isn't. Can you help with that?

@targos
Copy link
Member Author

targos commented Sep 25, 2023

@StefanStojanovic We're coming closer to Node.js 21, and I don't know who else to ask.

@mhdawson
Copy link
Member

Adding mention to @joaocgreis in case he can help out or know when @StefanStojanovic might be around.

@targos
Copy link
Member Author

targos commented Oct 2, 2023

We are now only two weeks before the release of Node.js 21. I think this is urgent.

@StefanStojanovic
Copy link
Contributor

Sorry for jumping in late, I had some issues that are now over and I'm available to help here going forward. Hey @targos just so I do not lose any action item out of sight - what tasks do you need help with in the CI exactly?

In test CI we have 6 Win2022-VS2022 machines which are fully integrated into the compilation job. Here I think we are good to go.

In release CI we have 3 Win2022-VS2022 machines. They are fully configured (with the vs2022 labels), but iojs+release is still not changed (I'll move this forward). Besides changes to that job and changes in the version selector script, do you think something else will be needed?

@targos
Copy link
Member Author

targos commented Oct 4, 2023

Thanks for coming back to this!

In test CI we have 6 Win2022-VS2022 machines which are fully integrated into the compilation job. Here I think we are good to go.

Agreed.

In release CI we have 3 Win2022-VS2022 machines. They are fully configured (with the vs2022 labels), but iojs+release is still not changed (I'll move this forward).

Changing iojs+release is what I had in mind.

Besides changes to that job and changes in the version selector script, do you think something else will be needed?

I don't think so.

@StefanStojanovic
Copy link
Contributor

I've created a test job to try these changes in the CI. From what I see VS2019 is excluded from v21 onward. However, we need to add [ /vs2022/, releaseType, lt(21) ] to prevent breaking the rule of a single VS version building each Node.js version.

Other than this, we can remove all VS2015/VS2017 and also make the test CI match the release CI (currently V20 is compiled on both VS2019 and VS2022 which I made as a temporary thing until VS2022 is enabled in release CI). However, all of these other changes are not that important right now, I can do it all in a following PR after this lands.

@targos if there's something else you could use my help with, please let me know.

@targos
Copy link
Member Author

targos commented Oct 5, 2023

@StefanStojanovic I added a skip of vs2022 for lt(21). Can you explicitly review the PR?

Copy link
Contributor

@StefanStojanovic StefanStojanovic left a comment

Choose a reason for hiding this comment

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

LGTM. Release builds now pick the correct VS versions. If I was nitpicking, I'd say Windows Version for Official Releases should be changed from 2012 R2 to 2019, but since I'll be doing a follow on PR anyway I'll change that and you can go ahead and land this as is @targos 👍

@targos targos merged commit c0a1bd0 into nodejs:main Oct 7, 2023
@targos targos deleted the vs2022 branch October 7, 2023 06:59
@StefanStojanovic
Copy link
Contributor

@targos FYI I've changed iojs+release to use vs2022 labels now. VS2022 should be used for v21+ from now on.

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

Successfully merging this pull request may close these issues.

4 participants