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

Modernize Windows Modern and Legacy Jenkins CI #4767

Closed
wants to merge 7 commits into from

Conversation

dilijev
Copy link
Contributor

@dilijev dilijev commented Mar 1, 2018

Modern config:

  • Server 2016 (a recent-ish build ~= RS2, RS3)
  • VS 2017 (Dev15)

Legacy config:

  • Server 2008 R2 (Windows 7)
  • VS 2015 (Dev14)

See related issues and PRs:

Fixes #4566: Update build configs (legacy -> dev14)

@dilijev dilijev changed the base branch from master to release/1.8 March 1, 2018 23:28
@dilijev dilijev changed the title Modernize CI Modernize Windows Modern and Legacy Jenkins CI Mar 1, 2018
@dilijev
Copy link
Contributor Author

dilijev commented Mar 1, 2018

@dilijev
Copy link
Contributor Author

dilijev commented Mar 1, 2018

@dotnet-bot test ci please

@dilijev
Copy link
Contributor Author

dilijev commented Mar 1, 2018

After the generator job runs I'll add a test PR against this branch.

@MSLaguana
Copy link
Contributor

Generator hit an error:
15:33:52 ERROR: (netci.groovy, line 234) No signature of method: netci$_run_closure2.call() is applicable for argument types: (java.lang.String, java.lang.String, null, java.lang.String, java.lang.Boolean, null, null) values: [Windows_NT, latest-dev15-5, null, -winBlue, true, null, null]

netci.groovy Outdated
@@ -221,34 +231,45 @@ def CreateStyleCheckTasks = { taskString, taskName, checkName ->
// INNER LOOP TASKS
// ----------------

CreateBuildTasks('Windows_NT', null, null, "-winBlue", true, null, null)
CreateBuildTasks(latestWindowsMachine, latestWindowsMachineTag, null, "-winBlue", true, null, null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah; you removed an additional null that you didn't need to. Needs a null after latestWindowsMachineTag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good catch, thanks.

@dilijev
Copy link
Contributor Author

dilijev commented Mar 1, 2018

@dotnet-bot test ci please

@jackhorton
Copy link
Contributor

Ill look more later, but any reason for targeting this on release/1.8-ci?

@dilijev
Copy link
Contributor Author

dilijev commented Mar 1, 2018

Legacy builds were dropped from 1.8 with our drop of Dev12 support. Figured we should add the new legacy checks here to pick up the lineage (and it would be good to know if this effectively not-covered release of ChakraCore is actually broken on Win 7).

@dilijev dilijev mentioned this pull request Mar 1, 2018
@mmitche
Copy link
Contributor

mmitche commented Mar 2, 2018

@dotnet-bot test OSX static_osx_osx_debug

@dilijev
Copy link
Contributor Author

dilijev commented Mar 2, 2018

@dotnet-bot
test OSX static_osx_osx_debug
test OSX static_osx_osx_test

@mmitche
Copy link
Contributor

mmitche commented Mar 2, 2018

@dilijev We think there are some serious lab issues going on right now. The OSX jobs are going to fail again most likely.

@dilijev
Copy link
Contributor Author

dilijev commented Mar 2, 2018

@mmitche okay, I'll hold off triggering them again. They're not the focus of this change anyway.

Copy link
Contributor

@jackhorton jackhorton left a comment

Choose a reason for hiding this comment

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

I still think that the xplat builds are a bit over the top, but perhaps thats better for a different PR? Also, once this gets to master, I will probably add a single x64_debug build for server 2016 + ICU (which will eventually go away once all 2016 builds opt into ICU by default)

@@ -57,7 +60,7 @@ if exist "%MSBUILD_PATH%\msbuild.exe" (
goto :MSBuildFound
)

echo Dev14 not found, trying Dev12...
echo Dev14 not found, trying to locate Dev12...
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we still bother trying to find dev12?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

'Ubuntu16.04': 'Ubuntu',
'OSX': 'OSX'
]

def defaultMachineTag = 'latest-or-auto'
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: defaultWindowsMachineTag

netci.groovy Outdated
@@ -221,34 +231,45 @@ def CreateStyleCheckTasks = { taskString, taskName, checkName ->
// INNER LOOP TASKS
// ----------------

CreateBuildTasks('Windows_NT', null, null, "-winBlue", true, null, null)
CreateBuildTasks(latestWindowsMachine, latestWindowsMachineTag, null, null, "-winBlue", true, null, null)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we are supposed to be passing "-winBlue" for testExtra anymore. What does that flag even do? Enable WinBlue tests? Is the script so old that they weren't originally enabled by default?

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that this is passed in a bunch of different places and should be cleaned up. Maybe make the jenkins script pass the appropriate flag for wherever its running so we don't have to specify it here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think -winBlue might enable "not Windows 7" tests which should probably continue running. Could be wrong but seems like we should just remove the winBlue tags and disable some tests on Windows 7.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I'm not mistaken there are significant platform differences across Win7 (Server 2008 R2) Win8 (Server 2012) WinBlue (Server 2012 R2), and Win10 -- and we have test platform configurations and test tags reflecting all of these. Worth an audit on the test tags as a separate work item, but whether or not we currently use this, our tests still "depend" on this on the appropriate platform.

However, I'll make sure that the "testExtra" accurately reflects that platform that each leg will actually run on. If modern = Win10 (Server 2016) then "-winBlue" is incorrect.

netci.groovy Outdated
'Windows_NT', 'ci_lite', '"/p:BuildLite=true"', '-winBlue -lite', false, null, null)
latestWindowsMachine, latestWindowsMachineTag, 'ci_lite', '"/p:BuildLite=true"', '-winBlue -lite', false, null, null)
// x64_debug Legacy
CreateBuildTask(true, 'x64', 'debug',
Copy link
Contributor

Choose a reason for hiding this comment

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

So ci_lite and ci_disablejit don't get rolling builds. Is that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no need for rolling builds since they're intended to be daily builds. The PR-only tasks for these daily build configs were added as a minimal sanity check. This is by design. We can revisit this (or the general distinction of daily|rolling) as a separate discussion and work item if you think that's worthwhile.

Copy link
Contributor

@boingoing boingoing left a comment

Choose a reason for hiding this comment

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

LGTM

@dilijev dilijev self-assigned this Mar 27, 2018
@dilijev
Copy link
Contributor Author

dilijev commented Mar 27, 2018

@dotnet-bot test ci please

@dilijev
Copy link
Contributor Author

dilijev commented Mar 27, 2018

@MSLaguana @jackhorton @boingoing @Penguinwizzard That should cover all of the feedback. Thanks for the reviews!

@dilijev
Copy link
Contributor Author

dilijev commented Mar 27, 2018

@dotnet-bot test ci please

@dilijev
Copy link
Contributor Author

dilijev commented Mar 27, 2018

@dotnet-bot
test OSX _no_jit_shared_osx_osx_test
test OSX static_osx_osx_release
test OSX static_osx_osx_test

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

Successfully merging this pull request may close these issues.

6 participants