-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Use a fixed version of CoreFX for testing #8938
Use a fixed version of CoreFX for testing #8938
Conversation
/cc @wtgodbe @RussKeldorph @russellhadley @gkhanna79 Please do not merge this yet. I need to mark a CoreFX build of opensuse13.2_release as saved so the logic will pick this up. |
@dotnet-bot test ci please |
/cc @weshaggard, FYI. This gives CoreCLR some breathing room to react to the dev/eng changes. |
@ellismg Thanks for buying us time. FYI, we recently moved some corefx test logic out of the groovy. Presumably that would require a similar change. One the plus side, I believe it will be much easier to test adapting to the workflow changes than when the code was in groovy. /cc @adiaaida @jashook |
Yup, I will fix that up as well. If there are other places, point them out and I'll get things fixed up. |
9df33b0
to
a6bc2fe
Compare
@dotnet-bot test ci please I fixed up the python script by adding a new argument -fx_commit (which defaults to HEAD) that allows you to specify a specific commit to checkout before building CoreFX. I then threaded that through the netci.groovy file. I ran the run tests script locally to do a simple smoke test and it was doing the right thing when the argument is passed and when it isn't. |
It would be helpful for me if someone who knows a little more about the job generation can ensure the jobs that are not yet on the python script plan are generated as expected. I was able to validate that jobs jitstress/x64_checked_ubuntu_corefx_baseline are passing the right arguments to the python script, but I can't really understand from the netci groovy script what cases fall down the legacy paths of using code inside netci.groovy themselves... |
FYI: @RussKeldorph CoreFX is doing their merge today. This looks okay to go from my end, but it would be helpful to have another pair of eyes that is more familiar with the CoreCLR side of things... |
@@ -2067,7 +2069,9 @@ def static calculateBuildCommands(def newJob, def scenario, def branch, def isPR | |||
// Build and text corefx | |||
def workspaceRelativeFxRoot = "_/fx" | |||
def absoluteFxRoot = "\$WORKSPACE/${workspaceRelativeFxRoot}" | |||
buildCommands += "python \$WORKSPACE/tests/scripts/run-corefx-tests.py -arch ${arch} -build_type ${configuration} -fx_root ${absoluteFxRoot} -fx_branch ${branch} -env_script ${scriptFileName}" | |||
def coreFxSpecificCommit = (branch == 'master') ? '551fe49174378adcbf785c0ab12fc69355cef6e8' : 'HEAD' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to hard code this in the python since it's so hard to test groovy changes. No need to plumb this through the script command line. If you're rushed, you can go ahead, though. Sorry for the delay reviewing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the change.
CoreFX is going to be merging changes soon that will break how we consume them to do our testing. To give us time to react, we'll fix the version of the repository we build to a commit before the changes. We'll also download artifacts from a saved build (produced before the change took place) so the layout is as we expect. The issue tracking cleaning this up is #8937
a6bc2fe
to
396c957
Compare
@dotnet-bot test ci please |
CoreFX has already made the changes and it's causing some jobs in CI to fail, so I am merging this now. |
…nd-artifacts-for-ci Use a fixed version of CoreFX for testing Commit migrated from dotnet/coreclr@de31af9
CoreFX is going to be merging changes soon that will break how we
consume them to do our testing. To give us time to react, we'll fix
the version of the repository we build to a commit before the
changes. We'll also download artifacts from a saved build (produced
before the change took place) so the layout is as we expect.
The issue tracking cleaning this up is #8937