Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

release/3.1 add macOS11 to RID graph#42968

Merged
wfurt merged 4 commits intodotnet:release/3.1from
wfurt:macos11
Aug 19, 2020
Merged

release/3.1 add macOS11 to RID graph#42968
wfurt merged 4 commits intodotnet:release/3.1from
wfurt:macos11

Conversation

@wfurt
Copy link
Member

@wfurt wfurt commented Aug 18, 2020

This is port of 5.0 change

fixes #40479

@wfurt wfurt added this to the 3.1.x milestone Aug 18, 2020
@wfurt wfurt requested review from Anipik and ericstj August 18, 2020 07:26
@wfurt
Copy link
Member Author

wfurt commented Aug 18, 2020

test failure in System.Text.RegularExpressions is unrelated.

@Anipik Anipik added the Servicing-consider Issue for next servicing release review label Aug 18, 2020
<RuntimeGroup Include="osx">
<Parent>unix</Parent>
<Architectures>x64;arm64</Architectures>
<Versions>11;11.0</Versions>
Copy link
Member

Choose a reason for hiding this comment

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

Because you've made osx11 a separate entry it doesn't treat 11 as compatible with 10. Is that what we want?

Copy link
Member

Choose a reason for hiding this comment

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

A similar discussion occurred with Tizen here: dotnet/runtime#38832

Copy link
Member Author

Choose a reason for hiding this comment

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

can we make the x64 compatible while adding arm64 only for 11?
Or should we add arm64 for all versions even if we do not have implementation?
It seems like Tizien took the later approach, right?

Master change is dotnet/runtime#40644

Copy link
Member

Choose a reason for hiding this comment

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

Third option would be to add functionality to special case as I suggested. It's up to you to define what you think the best behavior is. I'm not sure treating them as incompatible is correct, but I don't know enough about Apple's versioning and how their SDK's / platform API changes release-to-release. Perhaps some folks like @bartonjs or @akoeplinger might have an opinion.

Copy link
Member Author

Choose a reason for hiding this comment

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

cc: @marek-safar since he opened the original issue. AFAK, we not use same bits build on 10.x and it seems to work find with few issues. I don't mind taking look at the generation but I'm not sure how do I ensure correctness.

Copy link
Member

Choose a reason for hiding this comment

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

RIDs are all about what is most useful rather than what is correct. You can always have a case where things won't work, but that's not a reason to make them incompatible: folks can always include two assets to deal with an incompatibility. If it is more useful to treat osx11 as compatible with osx10 (so that folks can reuse a single asset the majority of the time) then that is how we should represent it.

Copy link
Member

Choose a reason for hiding this comment

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

yes osx11 should definitely be compatible with osx10.

@wfurt
Copy link
Member Author

wfurt commented Aug 18, 2020

can I do something like @ericstj? e.g. make OSX parent of the new node.

     <RuntimeGroup Include="macos">
       <Parent>osx</Parent>
       <Architectures>x64;arm64</Architectures>
       <Versions>11;11.0</Versions>
     </RuntimeGroup>

@ericstj
Copy link
Member

ericstj commented Aug 18, 2020

<RuntimeGroup Include="macos">

This defines a new RID prefix macos. I don't think we want that. My feedback was around the osx11 RIDs including osx10.x RIDs in their fallback chain. Specifically observe the differences in the fallback chains between

"osx.10.15-x64": [
"osx.10.15-x64",
"osx.10.15",
"osx.10.14-x64",
"osx.10.14",
"osx.10.13-x64",
"osx.10.13",
"osx.10.12-x64",
"osx.10.12",
"osx.10.11-x64",
"osx.10.11",
"osx.10.10-x64",
"osx.10.10",
"osx-x64",
"osx",
"unix-x64",
"unix",
"any",
"base"
],

And
"osx.11.0-x64": [
"osx.11.0-x64",
"osx.11.0",
"osx.11-x64",
"osx.11",
"osx-x64",
"osx",
"unix-x64",
"unix",
"any",
"base"
],

This indicates that packages which use RIDs like osx10.n will no longer work with osx11.0. This may or may not be the right thing to do. I'm observing that its different than what we've done for every past version of osx, so we need to understand if that's right or not.

@wfurt
Copy link
Member Author

wfurt commented Aug 18, 2020

that make sense @ericstj . Let's keep them together e.g. compatible unless proven otherwise.
I'll craft separate PR to update master once this looks good.

<Architectures>x64</Architectures>
<Versions>10.10;10.11;10.12;10.13;10.14;10.15</Versions>
<Architectures>x64;arm64</Architectures>
<Versions>10.10;10.11;10.12;10.13;10.14;10.15;11;11.0</Versions>
Copy link
Member

Choose a reason for hiding this comment

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

Why are we adding both 11 and 11.0? Typically we only do one or the other because folks would expect them to be equivalent, however its actually directional (11.0 is compatible with 11, but 11 cannot use 11.0 assets). What value do we calculate?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is what @marek-safar asked for in original ticket.
As far as I can tell, dotnet still sees the build as 10.16

Runtime Environment:
 OS Name:     Mac OS X
 OS Version:  10.16
 OS Platform: Darwin
 RID:         osx-x64
 Base Path:   /Users/furt/Downloads/dotnet/sdk/5.0.100-rc.1.20418.3/

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 did more testing and OS report it self as 11.0

furt@macos-11 foo % sw_vers
ProductName:	macOS
ProductVersion:	11.0

So I removed the 11 to be consistent with the rest. (dotnet still shows 10.16)

Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

Looks good. I've filed https://github.com/dotnet/arcade/issues/5998 to track being able to do this without adding the arch rids for older versions.

@wfurt wfurt changed the title add macOS11 to RID graph release/3.1 add macOS11 to RID graph Aug 19, 2020
@wfurt wfurt merged commit d1252eb into dotnet:release/3.1 Aug 19, 2020
@danmoseley danmoseley added the Servicing-approved Approved for servicing release label Aug 24, 2020
@danmoseley danmoseley modified the milestones: 3.1.x, 3.1.7, 3.1.8 Aug 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-Infrastructure Servicing-approved Approved for servicing release Servicing-consider Issue for next servicing release review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants