Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Use a fixed version of CoreFX for testing #8938

Merged
merged 1 commit into from
Jan 13, 2017

Conversation

ellismg
Copy link

@ellismg ellismg commented Jan 13, 2017

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

@ellismg ellismg added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Jan 13, 2017
@ellismg
Copy link
Author

ellismg commented Jan 13, 2017

/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.

@ellismg
Copy link
Author

ellismg commented Jan 13, 2017

@dotnet-bot test ci please

@ellismg
Copy link
Author

ellismg commented Jan 13, 2017

/cc @weshaggard, FYI. This gives CoreCLR some breathing room to react to the dev/eng changes.

@RussKeldorph
Copy link

@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

@ellismg
Copy link
Author

ellismg commented Jan 13, 2017

Presumably that would require a similar change.

Yup, I will fix that up as well. If there are other places, point them out and I'll get things fixed up.

@ellismg ellismg force-pushed the fix-corefx-hash-and-artifacts-for-ci branch from 9df33b0 to a6bc2fe Compare January 13, 2017 02:41
@ellismg
Copy link
Author

ellismg commented Jan 13, 2017

@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.

@ellismg
Copy link
Author

ellismg commented Jan 13, 2017

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...

@ellismg ellismg removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Jan 13, 2017
@ellismg
Copy link
Author

ellismg commented Jan 13, 2017

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'

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.

Copy link
Author

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
@ellismg ellismg force-pushed the fix-corefx-hash-and-artifacts-for-ci branch from a6bc2fe to 396c957 Compare January 13, 2017 21:23
@ellismg
Copy link
Author

ellismg commented Jan 13, 2017

@dotnet-bot test ci please

@ellismg
Copy link
Author

ellismg commented Jan 13, 2017

CoreFX has already made the changes and it's causing some jobs in CI to fail, so I am merging this now.

@ellismg ellismg merged commit de31af9 into dotnet:master Jan 13, 2017
@karelz karelz modified the milestone: 2.0.0 Aug 28, 2017
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
…nd-artifacts-for-ci

Use a fixed version of CoreFX for testing

Commit migrated from dotnet/coreclr@de31af9
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants