-
Notifications
You must be signed in to change notification settings - Fork 136
Update SHAs for coreclr, corefx, core-setup and standard to pick up source-build fixes #313
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
Update SHAs for coreclr, corefx, core-setup and standard to pick up source-build fixes #313
Conversation
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.
Changes look reasonable, CI errors in CLI seem pretty strange though--like MSBuild.dll is being passed as a source file to the compiler.
@@ -4,7 +4,7 @@ | |||
<PropertyGroup> | |||
<BuildArguments>-ConfigurationGroup=$(Configuration) -PortableBuild=false -SkipTests=true </BuildArguments> | |||
<BuildArguments Condition="$(Platform.Contains('arm'))">$(BuildArguments) -TargetArchitecture=$(Platform) -DisableCrossgen=true -CrossBuild=true</BuildArguments> | |||
<BuildCommand>$(ProjectDirectory)/build$(ShellExtension) $(BuildArguments) -- /p:BuildDebPackage=false</BuildCommand> | |||
<BuildCommand>$(ProjectDirectory)/build$(ShellExtension) $(BuildArguments) -- /p:BuildDebPackage=false /p:BuildAllPackages=true</BuildCommand> |
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.
Why did we need to pass BuildAllPackages=true? I wouldn't expect that to be necessary.
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.
The CLI build in source-build failed because of 2 missing depencencies: Microsoft.Extensions.DependencyModel and Microsoft.DotNet.PlatformAbstractions. These are conditionally built based on this property.
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.
We intentionally don't build these in servicing, similar to corefx, unless we have changes we want to ship. It has been a while since I've thought about this but it feels like we would have a lot more failures higher up based on not building all the packages.
I guess for now this should work. For reference the corefx issue I filed a while back is #210.
…ld/source-build into updateCoreFxCoreSetupSHAs
0bcf512
to
d774da5
Compare
…ld.Runtime to fix sporadic issue with CI where a broken version of Microsoft.Build.Runtime.15.4.8 was being installed from the msbuild myget feed. Add cli-deps to NuGet.Config for cli update-dependencies. Add ExternalRestoreSource to repo build command for cli.
…ith BuildTest=false
We will temporarily disable the ARM CI build. This is ready to merge. |
Source-build specific fixes were made in corefx and core-setup. Move to the release/2.0.0 version of these to pick up the code changes.
fixes #311