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.

Sign up for GitHub

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

Jump to bottom

Optional stress dependencies & don't populate CORE_ROOT twice #36851

Merged
merged 4 commits into from
May 24, 2020
Merged

Optional stress dependencies & don't populate CORE_ROOT twice #36851

merged 4 commits into from
May 24, 2020

Conversation

trylek
Copy link
Member

@trylek trylek commented May 21, 2020

Bruce noticed that we're downloading and installing stress
runtime dependencies several times during build. I have added a new
option to suppress this behavior in build-test and I fixed the
invocations of build-test based on Bruce's analysis. I have also
patched the build scripts to avoid populating CORE_ROOT in
"copynativeonly" mode.

Thanks

Tomas

Fixes: #36797

/cc: @dotnet/runtime-infrastructure

@trylek
Optional stress dependencies & don't populate CORE_ROOT twice
3fa7ab3 
Bruce noticed that we're downloading and installing stress
runtime dependencies several times during build. I have added a new
option to suppress this behavior in build-test and I fixed the
invocations of build-test based on Bruce's analysis. I have also
patched the build scripts to avoid populating CORE_ROOT in
"copynativeonly" mode.

Thanks

Tomas

Fixes: dotnet#36797
@trylek trylek requested a review from jkoritzinsky May 21, 2020 23:37
@jaredpar jaredpar mentioned this pull request May 22, 2020
Closed
BruceForstall
BruceForstall requested changes May 22, 2020
View reviewed changes
Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

Looks like a problem with your change is that you changed the "copynativeonly" phase to not generate the layouts (good) (and maybe "copynativeonly" should imply "skipstressdependencies" without having to pass that argument as well?)

Then, the Core_Root needs to be created with the "buildtestwrappersonly" step. On Windows, build-test.cmd does this. But on Linux, build-test.sh does not (which actually makes more sense, given the argument is "...only").

src/coreclr/build-test.cmd Outdated
@@ -98,13 +99,16 @@ if /i "%1" == "checked" (set __BuildType=Checked&set processedArgs

if /i "%1" == "ci" (set __ArcadeScriptArgs="-ci"&set __ErrMsgPrefix=##vso[task.logissue type=error]&set processedArgs=!processedArgs! %1&shift&goto Arg_Loop)

if /i "%1" == "skipstressdependencies" (set __SkipStressDependencies=1&shift&goto Arg_Loop)
Copy link
Member

Choose a reason for hiding this comment

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

Don't you also need the &set processedArgs=!processedArgs! %1 magic?

src/coreclr/build-test.cmd Outdated
@@ -280,8 +287,6 @@ if errorlevel 1 (
exit /b 1
)

:skipnative
Copy link
Member

Choose a reason for hiding this comment

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

Why is this removed?

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 believe it's not used anywhere. Perhaps I'm just blind, are you aware of any goto or call statement referring to this label?

Copy link
Member

Choose a reason for hiding this comment

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

Just above at the top of this "section": "if defined __SkipNative goto skipnative"

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, my bad, thanks for pointing that out, for some reason I was under the impression I saw two instances of the label in the file. I probably need to buy myself stronger glasses :-). Will put back...

@BruceForstall
Copy link
Member

Note also that in the R2R case, the "Crossgen framework assemblies" step (which follows the steps you modified) also downloads the stress dependencies and generates Core_Root (again) (at least on Windows).

@trylek
Cleanly separate CORE_ROOT generation and framework crossgenning
c684a02 
I have added a new option for only crossgenning the framework and
I fixed the build scripts to use it. I have added a separate
pipeline step for populating CORE_ROOT so that it no longer needs
to party on other build steps. My longer term thinking is that
I would love to move most of this logic to MSBuild and then the
individual steps will just correspond to targets or properties
controlling the individual "test build pipeline" steps.

Thanks

Tomas
BruceForstall
BruceForstall approved these changes May 23, 2020
View reviewed changes
Copy link
Member

@BruceForstall BruceForstall left a comment

Choose a reason for hiding this comment

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

LGTM

@trylek
Copy link
Member Author

trylek commented May 24, 2020

The AllConfiguration libraries build error looks very similar to

#36285

I'm following up to figure out whether it regressed again.

@trylek trylek merged commit da86a69 into dotnet:dev/infrastructure May 24, 2020
@trylek trylek deleted the DuplicateStressDeps branch May 24, 2020 09:18
jashook pushed a commit to jashook/runtime that referenced this pull request May 26, 2020
@trylek
Optional stress dependencies & don't populate CORE_ROOT twice (dotnet…
c0c42b1 
…#36851)

Bruce noticed that we're downloading and installing stress
runtime dependencies several times during build. I have added a new
option to suppress this behavior in build-test and I fixed the
invocations of build-test based on Bruce's analysis. I have also
patched the build scripts to avoid populating CORE_ROOT in
"copynativeonly" mode.

Thanks

Tomas

Fixes: dotnet#36797
jashook pushed a commit that referenced this pull request Jun 4, 2020
@sdmaclea @trylek
@safern @akoeplinger @dotnet-maestro @AndyAyersMS @am11 @Gnbrkm41 @monojenkins @thaystg @ViktorHofer @AaronRobinsonMSFT @vcsjones @bartonjs @EgorBo @nattress @lambdageek @krwq @kunalspathak @CarolEidt @weitzhandler @stephentoub @layomia @danmoseley @eiriktsarpalis @jkoritzinsky @joperezr @flamencist @MihaZupan @NikolaMilosavljevic @steveisok @davidwrighton @buyaa-n @VSadov @lpereira @swaroop-sridhar @ManickaP @MichalStrehovsky @mangod9 @BruceForstall @alexischr @BrzVlad @alfredmyers @NickCraver @naricc @YohDeadfall @eerhardt @directhex @janvorli @Anipik @Marusyk @vargaz @Youssef1313 @benaadams @marek-safar @AntonLapounov @AraHaan @tmds @wfurt @Jozkee @sywhang @GrabYourPitchforks @davmason @UnityAlex @fanyang-mono @tannergooding @echesakov @radical @ivdiazsa @gbalykov @shadow-cs @ericstj @skill-bit @CoffeeFlux @sakno @saucecontrol @ww898 @jkotas @w-flo @PeterSolMS @grendello @lateralusX @eanova @mmitche @JoshSchreuder @cshung @erozenfeld @kjpou1 @0xfk0
Dev infra master merge1 (#37274)
9e09fe3 
* Build all managed coreclr tests on OSX in CI (#36253)

Remove concept of targetGeneric and targetSpecific

Add OSX corelcr managed tests builds to all pipes
Remove all other platform coreclr managed test build

Remove property `TestUnsupportedOutsideWindows`
Rename conditional `DisableProjectBuild` to `CLRTestTargetUnsupported`

Refactor tests to allow all managed tests to be built on OSX.

Split managed tests which based on target properties conditionally:
+ `<Compile Include/>`
+ `<DefineConstant/>`

This creates a separate test for each target configuration permutation.

Add `<CLRTestTargetUnsupported/>` conditions to select these at build and/or
run time

Append split tests with `_Target*` to identify intended test target

For tests which depend on target specific internal details od System.Private.Corlib,
expose a dummy implementation for unsupported targets.

Clean up

Remove <TraitTags/>
Remove <CLRTestNeedTarget/>
Remove managedOSXBuild
Remove managedTestBuildOsGroup
Remove managedTestBuildOsSubGroup

Use issues.target to disable expected failures

Some tests are configured to run on a targets with
specific charecteristics.

Use issues.targets to conditionally disable tests
not intended to run on all targets.

* Setup pr and batch runs for dev/infrsatructure (#36218) (#36299)

Co-authored-by: Jarret Shook <jashoo@microsoft.com>

* Add alias CoreClrTestBuildHost (#36256)

* Add alias CoreClrTestBuildHost

Intended to allow easy switching of test build host between OSX_x64 & Linux_x64
depending on availablility and reliability.

* Add missing issues.targets from #36253 (#36351)

* Add missing issues.targets from #36253

PR #36253 enabled building all tests on OSX. It was intended to include this list of
tests to disable on unsupported platforms. This was dropped as part of rebasing for
dev/infra branch.

Lack of CI on dev/infra allowed this to be missed.

* Revise issues.targets

* Add src/coreclr/tests/src/baseservices/typeequivalence/simple

* Remove #if Windows from DllImportPathTest

* Build scripts while copying native binaries (#36482)

* Build scripts while copying native binaries

Stop building test scripts during the managed build phase
Build after copying the native targets

Fixes issues relative to building scripts on OSX

* Fix superpmi script generation

* Cleanup issues.targets

* Keep 6 failing IJW tests

* Fix IJW when managed tests are built on OSX (#36711)

* Fix IJW when managed tests are built on OSX

- Refactor Interop.setting.targets to Directory.Build.{targets,props}
- Remove include Interop.setting.targets from interop projects rely on SDK including
    Directory.Build.* automatically
- Add new CopyInteropNativeRuntimeDependencies target for copying IJW dependencies
  * Add <Copy/> task
- Delete obsolete instruction in Interop/ReadMe.md

* Cleanup Readme.md

* Optional stress dependencies & don't populate CORE_ROOT twice (#36851)

Bruce noticed that we're downloading and installing stress
runtime dependencies several times during build. I have added a new
option to suppress this behavior in build-test and I fixed the
invocations of build-test based on Bruce's analysis. I have also
patched the build scripts to avoid populating CORE_ROOT in
"copynativeonly" mode.

Thanks

Tomas

Fixes: #36797

* Build test scripts whenever CopyNativeProjectBinaries builds (#37028)

* Fix pri0 test build.

* Clean pri1 test build (#37266)

* Clean pri1 test build

* Undo testing comment

* Make sure weakreferencetest does not run on unix

* Fixup WinRT proj

* Fix path

Co-authored-by: Steve MacLean <Steve.MacLean@microsoft.com>
Co-authored-by: Tomáš Rylek <trylek@microsoft.com>
Co-authored-by: Santiago Fernandez Madero <safern@microsoft.com>
Co-authored-by: Alexander Köplinger <alex.koeplinger@outlook.com>
Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Andy Ayers <andya@microsoft.com>
Co-authored-by: Adeel Mujahid <adeelbm@outlook.com>
Co-authored-by: Ganbarukamo41 <ganbarukamo@gmail.com>
Co-authored-by: monojenkins <jo.shields+jenkins@xamarin.com>
Co-authored-by: thaystg <thaystg@users.noreply.github.com>
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
Co-authored-by: Aaron Robinson <arobins@microsoft.com>
Co-authored-by: Kevin Jones <kevin@vcsjones.com>
Co-authored-by: Jeremy Barton <jbarton@microsoft.com>
Co-authored-by: Egor Bogatov <egorbo@gmail.com>
Co-authored-by: Simon Nattress <nattress@gmail.com>
Co-authored-by: Aleksey Kliger (λgeek) <alklig@microsoft.com>
Co-authored-by: Krzysztof Wicher <kwicher@microsoft.com>
Co-authored-by: Kunal Pathak <Kunal.Pathak@microsoft.com>
Co-authored-by: Carol Eidt <carol.eidt@microsoft.com>
Co-authored-by: Shimmy <2716316+weitzhandler@users.noreply.github.com>
Co-authored-by: Stephen Toub <stoub@microsoft.com>
Co-authored-by: Layomi Akinrinade <laakinri@microsoft.com>
Co-authored-by: Dan Moseley <danmose@microsoft.com>
Co-authored-by: Eirik Tsarpalis <eirik.tsarpalis@gmail.com>
Co-authored-by: Jeremy Koritzinsky <jekoritz@microsoft.com>
Co-authored-by: Jose Perez Rodriguez <joperezr@microsoft.com>
Co-authored-by: Alexander Chermyanin <flamencist@mail.ru>
Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com>
Co-authored-by: Sergey Andreenko <seandree@microsoft.com>
Co-authored-by: Nikola Milosavljevic <nikolam@microsoft.com>
Co-authored-by: Steve Pfister <steveisok@users.noreply.github.com>
Co-authored-by: David Wrighton <davidwr@microsoft.com>
Co-authored-by: buyaa-n <bunamnan@microsoft.com>
Co-authored-by: Krzysztof Wicher <mordotymoja@gmail.com>
Co-authored-by: Vladimir Sadov <vsadov@microsoft.com>
Co-authored-by: Leandro Pereira <leandro.pereira@microsoft.com>
Co-authored-by: Swaroop Sridhar <swaroop.sridhar@microsoft.com>
Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com>
Co-authored-by: Michal Strehovský <MichalStrehovsky@users.noreply.github.com>
Co-authored-by: Manish Godse <61718172+mangod9@users.noreply.github.com>
Co-authored-by: Bruce Forstall <brucefo@microsoft.com>
Co-authored-by: Alexis Christoforides <alexis@thenull.net>
Co-authored-by: BrzVlad <BrzVlad@users.noreply.github.com>
Co-authored-by: Alfred Myers <git@alfredmyers.com>
Co-authored-by: Nick Craver <nrcraver@gmail.com>
Co-authored-by: Nathan Ricci <naricc@microsoft.com>
Co-authored-by: Thays Grazia <thaystg@gmail.com>
Co-authored-by: Yoh Deadfall <yoh.deadfall@hotmail.com>
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
Co-authored-by: Jo Shields <directhex@apebox.org>
Co-authored-by: Jan Vorlicek <janvorli@microsoft.com>
Co-authored-by: Anirudh Agnihotry <anirudhagnihotry098@gmail.com>
Co-authored-by: Roman Marusyk <Marusyk@users.noreply.github.com>
Co-authored-by: Zoltan Varga <vargaz@gmail.com>
Co-authored-by: Youssef Victor <31348972+Youssef1313@users.noreply.github.com>
Co-authored-by: Ben Adams <thundercat@illyriad.co.uk>
Co-authored-by: Marek Safar <marek.safar@gmail.com>
Co-authored-by: Anton Lapounov <antonl@microsoft.com>
Co-authored-by: AraHaan <15173749+AraHaan@users.noreply.github.com>
Co-authored-by: Tom Deseyn <tom.deseyn@gmail.com>
Co-authored-by: Tomas Weinfurt <tweinfurt@yahoo.com>
Co-authored-by: Tomas Weinfurt <furt@Shining.local>
Co-authored-by: David Cantu <dacantu@microsoft.com>
Co-authored-by: Sung Yoon Whang <suwhang@microsoft.com>
Co-authored-by: Levi Broderick <GrabYourPitchforks@users.noreply.github.com>
Co-authored-by: vargaz <vargaz@users.noreply.github.com>
Co-authored-by: David Mason <davmason@microsoft.com>
Co-authored-by: UnityAlex <UnityAlex@users.noreply.github.com>
Co-authored-by: Fan Yang <52458914+fanyang-mono@users.noreply.github.com>
Co-authored-by: Tanner Gooding <tagoo@outlook.com>
Co-authored-by: Egor Chesakov <Egor.Chesakov@microsoft.com>
Co-authored-by: radical <radical@users.noreply.github.com>
Co-authored-by: Ivan Diaz Sanchez <ivdiazsa@microsoft.com>
Co-authored-by: Gleb Balykov <g.balykov@samsung.com>
Co-authored-by: Honza Rameš <ramejan@gmail.com>
Co-authored-by: Eric StJohn <ericstj@microsoft.com>
Co-authored-by: Ivan <ivanbuha@outlook.com>
Co-authored-by: Ryan Lucia <rylucia@microsoft.com>
Co-authored-by: SRV <roman.sakno@gmail.com>
Co-authored-by: Clinton Ingram <clinton.ingram@outlook.com>
Co-authored-by: Mikhail Pilin <ww898@users.noreply.github.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
Co-authored-by: w-flo <w-flo@users.noreply.github.com>
Co-authored-by: Peter Sollich <petersol@microsoft.com>
Co-authored-by: grendello <grendello@users.noreply.github.com>
Co-authored-by: Johan Lorensson <lateralusx.github@gmail.com>
Co-authored-by: Erhan Atesoglu <47518605+eanova@users.noreply.github.com>
Co-authored-by: Matt Mitchell <mmitche@microsoft.com>
Co-authored-by: Josh Schreuder <joshschreuder@gmail.com>
Co-authored-by: Josh Schreuder <josh.schreuder@gmail.com>
Co-authored-by: Andrew Au <andrewau@microsoft.com>
Co-authored-by: Eugene Rozenfeld <erozen@microsoft.com>
Co-authored-by: Vlad Brezae <brezaevlad@gmail.com>
Co-authored-by: Kenneth Pouncey <kjpou@pt.lu>
Co-authored-by: Kirill Frolov <k.frolov@samsung.com>
Co-authored-by: Tomas Weinfurt <furt@DESKTOP-0J9757E.corp.microsoft.com>
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jkoritzinsky jkoritzinsky jkoritzinsky approved these changes

@jashook jashook jashook approved these changes

@BruceForstall BruceForstall BruceForstall approved these changes

@safern safern safern approved these changes

@ViktorHofer ViktorHofer Awaiting requested review from ViktorHofer

Assignees
No one assigned
Labels
area-Infrastructure-coreclr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@trylek @BruceForstall @jkoritzinsky @jashook @safern