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

Fix installer build on Alpine #48505

Merged
merged 2 commits into from
Feb 23, 2021
Merged

Fix installer build on Alpine #48505

merged 2 commits into from
Feb 23, 2021

Conversation

am11
Copy link
Member

@am11 am11 commented Feb 19, 2021

When building for linux-musl on Alpine Linux host, the current approach is to pass /p:OutputRid=linux-musl-{arch} argument to ./build.sh command for sfxprojs in installer, which use Microsoft.DotNet.SharedFramework.Sdk and resolve RuntimeIdentifer as linux-x64, rather than linux-musl-x64.

PR fixes it by deleting the third OS list (_outputRID). Few issues highlighted after this simplification are also fixed; missing /p:CrossBuild in src/tests restore projects.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@am11 am11 marked this pull request as ready for review February 19, 2021 20:15
@am11
Copy link
Member Author

am11 commented Feb 19, 2021

cc @ViktorHofer, now we have two OS lists instead of three.. it is gradually getting improved. 🙂

@@ -2,7 +2,7 @@

<PropertyGroup>
<_targetFrameworkVersionIndex>$(TargetFramework.IndexOfAny(".-0123456789"))</_targetFrameworkVersionIndex>
<_targetFrameworkIdentifier Condition="'$(_runtimeOSVersionIndex)' != '-1'">$(TargetFramework.SubString(0, $(_targetFrameworkVersionIndex)))</_targetFrameworkIdentifier>
<_targetFrameworkIdentifier Condition="'$(_targetFrameworkVersionIndex)' != '-1'">$(TargetFramework.SubString(0, $(_targetFrameworkVersionIndex)))</_targetFrameworkIdentifier>
Copy link
Member Author

Choose a reason for hiding this comment

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

This was a 4 years old typo, which seems to be working by accident.

src/tests/build.sh Outdated Show resolved Hide resolved
@ViktorHofer
Copy link
Member

cc @ViktorHofer, now we have two OS lists instead of three.. it is gradually getting improved. 🙂

Great to see, thanks so much. What's the remaining OS duplication?

@ViktorHofer
Copy link
Member

Could you please verify that with this change VS still builds as expected? Just to double that check that we don't regress.

Co-authored-by: Viktor Hofer <[email protected]>
@am11
Copy link
Member Author

am11 commented Feb 20, 2021

What's the remaining OS duplication?

Currently the matrix has the following concepts (I might have missed something):

OSes:

  1. _hostOS: local property for host OS
  2. TargetOS: public property for TargetOS, it defaults to _hostOS
  3. _portableOS: local property that mainly varies for portable, non-portable builds
  4. RuntimeOS: public property for computing runtime assets' path

RIDs:

  1. __DistroRID: this is computed by shell script (eng/native/init-distro-rid) and used to parse out the default value of RuntimeOS
  2. PackageRID: public property to restore packages. It defaults to {_hostOS}-{_hostArch} when building for host and {_portableOS}-{TargetArchtecture} or {RuntimeOS}-{TargetArchitecture}.
  3. OutputRid: public property for build packages, it defaults to PackageRID
  4. MicrosoftNetCoreIlasmPackageRuntimeId: public property that is computed based on several conditions; mainly rely on the logic of 'closest package we can use on host to build runtime tools for the given target'.

If we could reduce the number of properties, in the spirit of avoiding multiple ways of doing the same thing, it would make it make it more scalable and easier to comprehend. In particular, RuntimeOS, __DistroRID and __portableOS can be simplified/refactored, if we hash out the details of what precisely we are supporting and define a strong contract for each of the following use-case:

  • Portable build: ./build.sh - applies to platform with the concept of distros, host and target are same, produced packages have platform name (e.g. linux-arm64 or illumos-x64).
  • Portable cross build: ./build.sh -cross -os SomeUnix or ./build.sh -arch <something different than host>- host and target are different, so tooling will cross compile
  • Non-portable build: ./build.sh --portablebuild false - produced packages have distro name (e.g. ubuntu-arm64 or openindiana-x64). This is currently used by dotnet/source-build and Tizen folks
  • Non-portable cross build: ./build.sh -cross -os SomeDistro --portable false - this is currently not used by anyone (I think), but we have support for it.

TL;DR, ideally we just want to know two simple things: host and target .. but that is most likely an oversimplification.. 🤣

One issue is the lack of Tizen and non-portable / source-build CI legs, which results in a lots of 'what if someone is using it' kind of cautions and hence the tech debt.

@am11
Copy link
Member Author

am11 commented Feb 20, 2021

Could you please verify that with this change VS still builds as expected? Just to double that check that we don't regress.

I followed these steps https:/dotnet/runtime/blob/d4d50db6768f729857b27ced3b68067bf30d18de/docs/workflow/building/libraries/README.md#quick-start and built System.Text.RegularExpressions and System.Globalization solutions inside Visual Studio, all projects succeeded.

@ViktorHofer
Copy link
Member

Maybe we should move that discussion into a separate issues and loop more folks in?

@ViktorHofer
Copy link
Member

Based on the recent infra changes let's retrigger a build and then merge :)

@ViktorHofer
Copy link
Member

/azp run runtime

@ViktorHofer
Copy link
Member

/azp run runtime-dev-innerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ViktorHofer ViktorHofer merged commit c9fe433 into dotnet:master Feb 23, 2021
@am11 am11 deleted the feature/alpine/build-fixes branch February 23, 2021 09:10
ViktorHofer added a commit that referenced this pull request Feb 23, 2021
ViktorHofer added a commit that referenced this pull request Feb 23, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Mar 25, 2021
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.

2 participants