Using Versionless rids instead of custom OSes by Anipik · Pull Request #33315 · dotnet/runtime

Anipik · 2020-03-06T23:56:22Z

ericstj

did a scan of libraries + plumbing and it seemed ok. I want to make sure we have folks from coreclr and installer review as well. This one may warrant a manual official build.

jkotas · 2020-03-07T01:36:30Z

The abbreviated and all lower identifiers look odd when everything else in .NET is non-abbreviated and CamelCased.

ericstj · 2020-03-07T18:35:00Z

@jkotas the same could be said for TargetFrameworks. These come from the same source.

jkotas · 2020-03-07T19:45:39Z

@@ -301,7 +258,7 @@
    <!-- Helix properties -->
    <OSPlatformConfig>$(TargetOS).$(Platform).$(Configuration)</OSPlatformConfig>
    <AnyOSPlatformConfig>AnyOS.AnyCPU.$(Configuration)</AnyOSPlatformConfig>


Lower case AnyOS and AnyCPU as well?

AnyCPU comes from VS and this is the casing they use.
AnyOS is our invention, and perhaps it should be eliminated. There is a representation in the RID graph:

runtime/src/libraries/pkg/Microsoft.NETCore.Platforms/runtime.json

Line 370 in 1124c1a

"any": {

I'd first question any place that needed to actually know this string. That feels like it could be an artifact of the old config system.

ViktorHofer

Please revert changes in eng/common

ViktorHofer · 2020-03-08T17:50:37Z

          platforms: ${{ parameters.platforms }}
        helixQueueGroup: ${{ parameters.helixQueueGroup }}
-        managedTestBuildOsGroup: Linux
+        managedTestBuildOsGroup: linux


@Anipik did we miss these when you renamed OSGroup to BuildOS?

I didnt change the .yml scripts variable names in the previous PR. i Wanted to keep that little bit separate as other change was getting crowded.

akoeplinger · 2020-03-09T20:01:13Z

    endif()
 else()
-    if(NOT (CLR_CMAKE_HOST_OS STREQUAL Windows_NT))
+    if(NOT (CLR_CMAKE_HOST_OS STREQUAL win))


All the other CLR_CMAKE_HOST_OS values on other platforms are initialized to longer names that start with upper case so this makes it inconsistent.

You could change it to Windows on L117 instead.

Anipik · 2020-03-17T15:25:51Z

@ericstj @ViktorHofer @safern can you take a look at this. Most of the failures are resolved here and i have a pr in jitutils to fix the formatting failure.

ericstj · 2020-03-17T21:34:54Z

@@ -81,6 +81,10 @@
      <Versions>1;2</Versions>
    </RuntimeGroup>

+    <RuntimeGroup Include="WebAssembly">


This is a product change, did you mean to include this? I thought this was still under discussion: #33327

jkotas · 2020-03-18T03:38:27Z

    set(CLR_CMAKE_HOST_UNIX_AMD64 1)
    set(CLR_CMAKE_HOST_DARWIN 1)
    set(CMAKE_ASM_COMPILE_OBJECT "${CMAKE_C_COMPILER} <FLAGS> <DEFINES> <INCLUDES> -o <OBJECT> -c <SOURCE>")
+    set(CLR_CMAKE_HOST_OS osx)


What is the convention for CLR_CMAKE_HOST_OS ? This line has it lower cased RID, and the very next line has it upped cased human readable?

Again, this feels pretty unnatural to use the lower-cased shortened OS names in CMake. The convention in CMake is to use Linux, Windows, etc.

jkotas · 2020-03-18T04:03:58Z

I am sorry, but I still do not understand the benefit of this change. The RID OS names everywhere look plain ugly, we still have the good names in many places and so this is just moving the inconsistency to a different spot. I understand that the installers have to figure out the RID, and this change will make it a bit simpler. How many lines are we going to save by doing this? Are there bad problems with this from the past that this is going to avoid?

BruceForstall · 2020-03-18T05:02:32Z

@Anipik Can you explain in the initial comment above what the PR title actually means, and what its implication is for the various components, including dev workflow? The PR title and associated "Fixed" issue are not sufficiently detailed for me to understand what this is about.

BruceForstall · 2020-03-18T05:26:27Z

Looking through the change, this looks like an extremely disruptive change. There will be many individual scripts (outside the repo) that need to be adjusted. It also seems to turn RID concepts into user-visible concepts, possibly unnecessarily. I always disliked the usage of "Windows_NT" (IMO, it should be "Windows" - we stopped saying "NT" like 20 years ago), and "OSX" should be "macOS". But using "win" everywhere that an OS name is required seems like overkill. Especially since "win" is an English word as well as a magic RID abbreviation meaning the Windows OS. Using "win" instead of "Windows" in non-RID contexts seems wrong. Is this whole change worth the trouble (and inevitable long tail fallout)?

ericstj · 2020-03-18T21:22:34Z

The benefit here is that we change the cross-targeting parameter for the libraries projects to match the actual mechanism that is used to select libraries and our runtime implementations (RID). This concept and value space is something built into our product, as opposed to the current set of values which is something only that's really just set of strings we use in comparisons with a somewhat organic history.

I agree that this is a pretty huge change touching more than just libraries. There is some benefit to reducing the number of unique concepts and manual mappings that need to be understood. I believe the idea here was that after this diff folks could think about a runtime in a single language / value space. Initially I was just planning on taking this to our libraries infra, but @Anipik generalized to the entire repo in order to minimize the number of "OS" concepts we had floating around.

Anipik · 2020-03-19T17:38:04Z

@jkotas @BruceForstall @ericstj what is the final agreement here ?

jkotas · 2020-03-20T03:19:48Z

My take:

Changes that affect developer workflow should be synchronized using Infrastructure - Rollout (April 2020) #33821
For anything that gets renamed to use RIDs, make it follow RIDs 100%: i.e.: Debug\win-x64 instead of win.x64.Debug, linux-arm instead linux_arm, etc.
Renaming of the build output directories to use RIDs sounds reasonable. I agree with Eric that the .NET SDK uses RID based directory names, and so it sounds reasonable for consistency.
Can this be broken into multiple changes? E.g. Would it make sense to rename the output directories as the step 1, without changing the other stuff? Or can the renames in the CI config be separate change?
The non-RID names are used in number of places, and we won't be able to change all of them and achive 100% consistency. I do not think it makes sense to try to change as much as possible to use RID OS names. I think we should rather strike the right ballance, e.g. switch to use RIDs where there is a clear prior art (e.g. where the .NET SDK is using them) and leave the rest alone. For example, I do not think it makes sense to be changing the CMakeFiles to use RID OS names.

Anipik · 2020-03-20T17:05:54Z

@jkotas that looks like a good plan. i will start working on it and try to split in multiple chunks

Anipik added 5 commits March 6, 2020 15:51

removing from coreclr

564a523

windows_NT -> win from libraries

4721bce

build all conig for libraries

cd6d70a

remove windows_nt from installer

50eda63

renaming in other files

7db1d6b

Anipik requested review from ViktorHofer, ericstj and safern March 6, 2020 23:56

Anipik requested a review from ahsonkhan as a code owner March 6, 2020 23:56

Dotnet-GitSync-Bot added the area-Infrastructure-libraries label Mar 6, 2020

jkoritzinsky added area-Infrastructure and removed area-Infrastructure-libraries labels Mar 7, 2020

ericstj reviewed Mar 7, 2020

View reviewed changes

Comment thread src/libraries/Directory.Build.props Outdated

ericstj requested review from dagood and removed request for ahsonkhan March 7, 2020 00:11

ericstj approved these changes Mar 7, 2020

View reviewed changes

jkotas reviewed Mar 7, 2020

View reviewed changes

Comment thread eng/pipelines/common/xplat-setup.yml

jkotas reviewed Mar 7, 2020

View reviewed changes

ViktorHofer reviewed Mar 8, 2020

View reviewed changes

Comment thread eng/common/templates/job/job.yml Outdated

ViktorHofer requested changes Mar 8, 2020

View reviewed changes

ViktorHofer reviewed Mar 8, 2020

View reviewed changes

Comment thread src/libraries/Directory.Build.props

Anipik added 2 commits March 9, 2020 12:11

file name to lowercase change

bc24e64

fixing the build-runtime.cmd failure

b3e9a30

akoeplinger requested changes Mar 9, 2020

View reviewed changes

jkotas reviewed Mar 9, 2020

View reviewed changes

Comment thread ...ler/pkg/projects/Microsoft.NETCore.DotNetHost/runtime.osx.Microsoft.NETCore.DotNetHost.props

Anipik added 2 commits March 9, 2020 14:36

comments and casing uname casing corrected

097b0a9

fixing the mono build

5ad264a

Anipik added 3 commits March 13, 2020 14:26

addressing comments

99ccef6

adding missing dollar sign

6eb7ad3

Merge branch 'master' into Rid

0b5d7eb

safern reviewed Mar 16, 2020

View reviewed changes

Comment thread eng/pipelines/coreclr/templates/run-performance-job.yml Outdated

Anipik added 2 commits March 16, 2020 10:11

fix performance test

3df8242

Merge branch 'master' into Rid

fd1ac99

Anipik mentioned this pull request Mar 17, 2020

Replacing Custom Oses with versionless rids dotnet/jitutils#254

Closed

Merge branch 'master' into Rid

209e617

ericstj reviewed Mar 17, 2020

View reviewed changes

ericstj requested review from NikolaMilosavljevic, ViktorHofer and akoeplinger March 17, 2020 21:36

Anipik added 2 commits March 17, 2020 18:53

fix formating leg with recent change

35f7e8c

Merge branch 'master' into Rid

8879132

Anipik requested review from ericstj, jkotas and safern March 18, 2020 02:04

jkotas reviewed Mar 18, 2020

View reviewed changes

Anipik closed this Mar 20, 2020

Anipik deleted the Rid branch March 27, 2020 21:46

Anipik mentioned this pull request May 27, 2020

Use RID-layout for OS configurations in libraries #32451

Closed

ghost locked as resolved and limited conversation to collaborators Dec 10, 2020

Uh oh!

Conversation

Anipik commented Mar 6, 2020

Uh oh!

Uh oh!

ericstj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkotas commented Mar 7, 2020

Uh oh!

ericstj commented Mar 7, 2020

Uh oh!

jkotas Mar 7, 2020

Choose a reason for hiding this comment

Uh oh!

ericstj Mar 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ViktorHofer left a comment

Choose a reason for hiding this comment

Uh oh!

ViktorHofer Mar 8, 2020

Choose a reason for hiding this comment

Uh oh!

Anipik Mar 9, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

akoeplinger Mar 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Anipik commented Mar 17, 2020

Uh oh!

ericstj Mar 17, 2020

Choose a reason for hiding this comment

Uh oh!

jkotas Mar 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Mar 18, 2020

Choose a reason for hiding this comment

Uh oh!

jkotas commented Mar 18, 2020

Uh oh!

BruceForstall commented Mar 18, 2020

Uh oh!

BruceForstall commented Mar 18, 2020

Uh oh!

ericstj commented Mar 18, 2020

Uh oh!

Anipik commented Mar 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkotas commented Mar 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Anipik commented Mar 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

ericstj Mar 17, 2020 •

edited

Loading

akoeplinger Mar 9, 2020 •

edited

Loading

jkotas Mar 18, 2020 •

edited

Loading

Anipik commented Mar 19, 2020 •

edited

Loading

jkotas commented Mar 20, 2020 •

edited

Loading