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 rid processing on macOS #60494

Merged
merged 5 commits into from
Oct 19, 2021
Merged

fix rid processing on macOS #60494

merged 5 commits into from
Oct 19, 2021

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Oct 15, 2021

#59066 made some changes how we calculate RID on macOS and it was not quite right.

The test was failing because there was embedded '\0'. So even if we add the architecture, that would be stripped with .c_str. It actually worked for 11.2.3 I started with but failed on 11.2 (no real ARM dependency IMHO)

There is still another problem. The way how the old code was written we would always return osx.11.0 from get_current_os_rid_platform even for 11.6. That does not break the test but then when we walk the graph we would end up with generic osx-arm64. So to maintain compatibility, I added special processing for 11.x (and verified with dotnet --info)

For 12.0+ the rid will be osx.12-arm64 and that match <Versions>10.10;10.11;10.12;10.13;10.14;10.15;10.16;11.0;12</Versions>

fixes #60341

@wfurt wfurt self-assigned this Oct 15, 2021
@ghost
Copy link

ghost commented Oct 15, 2021

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

#59066 made some changes how we calculate RID on macOS and it was not quite right.

The test was failing because there was embedded '\0'. So even if we add the architecture, that would be stripped with .c_str. It actually worked for 11.2.3 I started with but failed on 11.2 (no real ARM dependency IMHO)

There is still another problem. The way how the old code was written we would always return osx.11.0 from get_current_os_rid_platform even for 11.6. That does not break the test but then when we walk the graph we would end up with generic osx-arm64. So to maintain compatibility, I added special processing for 11.x (and verified with dotnet --info)

For 12.0+ the rid will be osx.12-arm64 and that match <Versions>10.10;10.11;10.12;10.13;10.14;10.15;10.16;11.0;12</Versions>

fixes #60341

Author: wfurt
Assignees: wfurt
Labels:

area-Host

Milestone: -

Copy link
Member

@vitek-karas vitek-karas 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.

src/native/corehost/hostmisc/pal.unix.cpp Outdated Show resolved Hide resolved
@danmoseley
Copy link
Member

Regression was after 6.0 fork right?

@agocke
Copy link
Member

agocke commented Oct 16, 2021

The test was failing because there was embedded '\0'.

I don't understand this -- where was the embedded '\0'?

@wfurt
Copy link
Member Author

wfurt commented Oct 18, 2021

I don't understand this -- where was the embedded '\0'?

This comes from @agocke

std::string release(str, size);
ridOS.append(_X("osx."));
ridOS.append(release);
}

With the current code and 11.6 the size is 5 while strlen is 4.
And then later on

rid = pal::get_current_os_rid_platform();
if (rid.empty() && use_fallback)
rid = pal::get_current_os_fallback_rid();
if (!rid.empty())
{
rid.append(_X("-"));
rid.append(get_arch());
}
return rid;

So it seems like if we construct std::string that from more than we need to then we can sendup with osx.11.6 when consumed in C e.g. the -arm64 is effectively ignored.

@wfurt
Copy link
Member Author

wfurt commented Oct 18, 2021

Regression was after 6.0 fork right?

yes. But we should take both to 6.0 - either now or via servicing as the original intent is to support macOS 12.

@wfurt
Copy link
Member Author

wfurt commented Oct 19, 2021

LLVM AOT looks like infrastructure unrelated to the change.

@wfurt wfurt merged commit 07fff0a into dotnet:main Oct 19, 2021
@wfurt wfurt deleted the macos12 branch October 19, 2021 16:50
size = pos - str;

}
else if (major == 12)
Copy link

Choose a reason for hiding this comment

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

Shouldn't this read else if (major == 11)? I think line 594 will never execute because, if major==12, line 587 executes instead.

Copy link
Contributor

@AustinWise AustinWise Oct 20, 2021

Choose a reason for hiding this comment

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

I was thinking that too. Here is a PR to fix: #60668

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, this should not be there. It somehow sneaked in when I was trying to remove the extra size calculation ;(

wfurt added a commit to wfurt/runtime that referenced this pull request Oct 30, 2021
* fix rid processing on macOS

* Update src/native/corehost/hostmisc/pal.unix.cpp

* Update src/native/corehost/hostmisc/pal.unix.cpp

* remove extra size calculation
Anipik pushed a commit that referenced this pull request Nov 10, 2021
* RID work for macOS 12 (#59066)

* fix rid processing on macOS (#60494)

* fix rid processing on macOS

* Update src/native/corehost/hostmisc/pal.unix.cpp

* Update src/native/corehost/hostmisc/pal.unix.cpp

* remove extra size calculation

* Fix "fix rid processing on macOS" (#60668)

The `else if (major == 12)` is dead code, since the previous if `if (major > 11)` would be true for `major == 12`. Judging by the comment and code, it looks like the intention of this `else if` statement was to match `major == 11`.

* add the packaging for platforms package

Co-authored-by: Austin Wise <[email protected]>
Co-authored-by: Anirudh Agnihotry <[email protected]>
@ghost ghost locked as resolved and limited conversation to collaborators Nov 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RuntimeIdentifierTests.VerifyOSRid started failing on macOS ARM64
8 participants