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

source-build: support building aspnetcore using non-portable runtime packages. #43937

Merged
merged 3 commits into from
Sep 29, 2022

Conversation

tmds
Copy link
Member

@tmds tmds commented Sep 13, 2022

Currently source-build performs 'runtime-portable' build that produces 'linux-{arch}' packages that are used when building ASP.NET Core.

With this change, we can use the non-portable packages that are produced by the source-build 'runtime' build, and eliminate the 'runtime-portable' build.

cc @omajid @MichaelSimons @crummel

@tmds tmds requested review from a team, dougbu, wtgodbe and Pilchie as code owners September 13, 2022 07:18
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Sep 13, 2022
@ghost
Copy link

ghost commented Sep 13, 2022

Thanks for your PR, @tmds. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@tmds tmds force-pushed the sb_non_portable branch 2 times, most recently from b464e91 to 8e9ec0c Compare September 13, 2022 08:12
…packages.

Currently source-build performs 'runtime-portable' build that produces 'linux-{arch}'
packages that are used when building ASP.NET Core.

With this change, we can use the non-portable packages that are produced by the
source-build 'runtime' build, and eliminate the 'runtime-portable' build.
@Pilchie Pilchie added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Sep 13, 2022
@ghost
Copy link

ghost commented Sep 13, 2022

Hey @dotnet/aspnet-build, looks like this PR is something you want to take a look at.

@dougbu
Copy link
Member

dougbu commented Sep 15, 2022

The CI build is just hitting a known area of flaky tests. However, that doesn't validate the '$(PortableBuild)' == 'false' case. Have you tested the new scenario somewhere❔

@dougbu
Copy link
Member

dougbu commented Sep 18, 2022

@MichaelSimons what are your thoughts on this PR and how to test it prior to merging❔ I think it should do what it says on the tin but am not confident.

@tmds
Copy link
Member Author

tmds commented Sep 20, 2022

@MichaelSimons what are your thoughts on this PR and how to test it prior to merginggrey_question I think it should do what it says on the tin but am not confident.

I created a PR in installer to help validate this change: dotnet/installer#14549.
And dotnet/source-build#3027 describes why the change is being made.

@dougbu
Copy link
Member

dougbu commented Sep 20, 2022

I created a PR in installer to help validate this change: dotnet/installer#14549.

Should we get this in now or wait for that PR to be merged❔

@MichaelSimons
Copy link
Member

Should we get this in now or wait for that PR to be merged❔

I am in favor of merging now.

@dougbu
Copy link
Member

dougbu commented Sep 27, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@dougbu dougbu enabled auto-merge (squash) September 27, 2022 17:06
@dougbu
Copy link
Member

dougbu commented Sep 27, 2022

Enabled auto-merge. New CI build should pick up some reliability fixes from our main branch and get this in today.

@@ -6,6 +6,8 @@
<TargetOsName Condition=" '$(TargetOsName)' == '' AND $([MSBuild]::IsOSPlatform('FreeBSD'))">freebsd</TargetOsName>
<TargetArchitecture Condition="'$(TargetArchitecture)' == ''">x64</TargetArchitecture>
<TargetRuntimeIdentifier Condition="'$(TargetRuntimeIdentifier)' == ''">$(TargetOsName)-$(TargetArchitecture)</TargetRuntimeIdentifier>
<PortableBuild Condition="'$(PortableBuild)' == ''">true</PortableBuild>
<DefaultAppHostRuntimeIdentifier Condition=" '$(PortableBuild)' == 'false' ">$(TargetRuntimeIdentifier)</DefaultAppHostRuntimeIdentifier>
Copy link
Member

Choose a reason for hiding this comment

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

Unsure what the existing pattern is but usually in the msbuild world when you want to mimic a boolean, you condition on something either being true or not being true. With conditions that assert on true and false you are allowing the property to have values that aren't covered by these conditions.

TL;DR: I would expect this to be '$(PortableBuild)' != 'true'

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 had first written it like that, and then inverted it in 82414df because of the build failures: https://github.com/dotnet/aspnetcore/runs/8326525459.

I rewrote the check that in case PortableBuild is not set, the checks default to portable behavior.

@ViktorHofer
Copy link
Member

@dougbu Auto merge doesn't seem to work for this PR

@dougbu dougbu merged commit 38d3043 into dotnet:main Sep 29, 2022
@ghost ghost added this to the 8.0-preview1 milestone Sep 29, 2022
@dougbu
Copy link
Member

dougbu commented Sep 29, 2022

Just needed an approval 😁

@tmds
Copy link
Member Author

tmds commented Nov 9, 2022

@dougbu can you initiate the backport to 7.0 for this? It has already been backported to 6.0 in #44752.

@ghost
Copy link

ghost commented Nov 9, 2022

Hi @tmds. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@dougbu
Copy link
Member

dougbu commented Nov 9, 2022

/backport to release/7.0

@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2022

Started backporting to release/7.0: https://github.com/dotnet/aspnetcore/actions/runs/3430831191

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants