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

Implement RID for SunOS-derived operating systems #37016

Merged
merged 4 commits into from
Jun 11, 2020
Merged

Implement RID for SunOS-derived operating systems #37016

merged 4 commits into from
Jun 11, 2020

Conversation

am11
Copy link
Member

@am11 am11 commented May 26, 2020

This PR implements:

  • distinction between Illumos and Oracle Solaris, which are two forks of OpenSolaris since 2010.
  • RID for Illumos and Solaris:
    • Solaris and Illumos derived from Unix.
    • OmniOS, OpenIndiana and SmartOS derived from Illumos.

A note for OpenIndiana: struct utsname members currently do not provide OS release information like other distros, so it is left version-less (similar to Gentoo). Additionally, we cannot rely on /etc/release/version etc. which do not contain consistent results across the zones.

Contributes to: #34944.

@ghost
Copy link

ghost commented May 26, 2020

Tagging subscribers to this area: @ViktorHofer
Notify danmosemsft if you want to be subscribed.

@am11
Copy link
Member Author

am11 commented May 26, 2020

cc @janvorli, @jclulow

@am11
Copy link
Member Author

am11 commented May 26, 2020

@janvorli, @wfurt, I have also fixed a FreeBSD case in pal.unix.cpp, where pos was unused in string::append. Please let me know if you agree with it.

@@ -52,7 +52,8 @@
<TargetOS Condition="'$(TargetOS)' == '' and $([MSBuild]::IsOSPlatform('OSX'))">OSX</TargetOS>
<TargetOS Condition="'$(TargetOS)' == '' and $([MSBuild]::IsOSPlatform('FREEBSD'))">FreeBSD</TargetOS>
<TargetOS Condition="'$(TargetOS)' == '' and $([MSBuild]::IsOSPlatform('NETBSD'))">NetBSD</TargetOS>
<TargetOS Condition="'$(TargetOS)' == '' and $([MSBuild]::IsOSPlatform('SUNOS'))">SunOS</TargetOS>
<TargetOS Condition="'$(TargetOS)' == '' and $([MSBuild]::IsOSPlatform('ILLUMOS'))">Illumos</TargetOS>
Copy link
Member

Choose a reason for hiding this comment

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

What was wrong with using SUNOS as the name for all the flavors?

Copy link
Member Author

Choose a reason for hiding this comment

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

Illumos and Oracle Solaris are two forks of OpenSolaris which were created in 2010.

Despite of many similaries, binaries compiled on an Illumos distro are not compatible with Oracle Solaris and vice versa. They differ by:

  • libc and its version; to begin with, dlopen/dlsym specially look for ILLUMOS xyz when we try to execute Illumos binary on Solaris.
  • supported syscalls; made by the libc itself (before the user code even begins).

Like Darwin is to macOS, iOS, tvOS etc. some utilities and tools such as uname, python and cmake still use the legacy SunOS name on both platforms for compatibility reasons. However, on Illumos uname -o returns illumos while Oracle Solaris does not support -o option.

Copy link
Member

Choose a reason for hiding this comment

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

binaries compiled on an Illumos distro are not compatible with Oracle Solaris and vice versa

The same is true e.g. between Ubuntu and Alpine. We do not have different TargetOS for these two.

Copy link
Member Author

Choose a reason for hiding this comment

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

True. On the other hand, their history is also similar to how OS X and FreeBSD used to share same base, but now they are diverged. Although there are some similarities in API usage, we consider them two different platforms on TargetOS level.

Copy link

Choose a reason for hiding this comment

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

If I am not mistaken modern binaries from Solaris 11+ don't run at all on illumos and the reverse is also true. Some older binaries from the Solaris 10 days might still work on both though.

Treating both the same has caused a lot of ugly hacks in Makefiles over the years, every few years there is also talk from dropping SunOS entirely from uname. But as of now that would break too many software that treat both the same.

For new languages (.e.g rust, go, ...) we are treating illumos seperate. Although IIRC for go illumos is a child of sunos (but I can't quickly find the page I was thinking of to confirm this)

@am11
Copy link
Member Author

am11 commented May 27, 2020

@jkotas, @janvorli, I have resolved merge conflicts and rebased with running master. Please take another look when you get a chance.

The idea here is to keep Illumos and Solaris as separate platforms in MSBuild scripts and dotnet code from the start; by avoiding SunOS concept (which was superseded by Solaris in mid 90's). We can change it if it is deemed suitable, and I do not feel strongly. Please let me know. This is just something I was recommended by community members to treat two platforms separate.

This PR serves as a prerequisite to the next step; cross-compilation via Ubuntu. I am trying to make progress and upstream changes in meaningful chunks sequentially.

@jkotas
Copy link
Member

jkotas commented May 27, 2020

@ericstj - RID graph changes

@jkotas jkotas requested a review from ericstj May 27, 2020 18:26
@ericstj ericstj requested a review from wfurt May 27, 2020 18:59
<Versions>11</Versions>
</RuntimeGroup>

<RuntimeGroup Include="illumos">
Copy link
Member

Choose a reason for hiding this comment

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

Will this be a something we leverage to build portable binaries (similar to linux-x64)? If not, should you account for such a RID and should solaris import it? cc @janvorli

Copy link
Member Author

@am11 am11 May 27, 2020

Choose a reason for hiding this comment

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

Tested with dotnet binary: when compiled on an illumos based distro, SmartOS, and copied artifacts/bin to other illumos distros: OpenIndiana and OmniOS it worked; but not on Oracle Solaris 11x.

Oracle Solaris from that perspective is a separate platform:

  • different syscalls supported by illumos and Solaris kernels.
  • they both have their own libc implementations; POSIX compliant but different, so the library loader (ld) looks for different versions and fails if we try to run binary compiled on one, ld fails on the other platform. If we satisfy it somehow (local copy of libc and set LD_LIBRARY_PATH to point to its directory), that fails when local libc makes unsupported syscall to kernel.

Both platforms have portable binaries for their own distros/versions, but they do not mix from derivation perspective.

illumos and Oracle Solaris were forked from OpenSolaris in 2010, since then they have diverged.
This is similar to, FreeBSD and OS X used to share same base once, but lately they have diverged.

Copy link

@sjorge sjorge May 27, 2020

Choose a reason for hiding this comment

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

See my comment here #37016 (comment), for new stuff they are almost certainly no longer binary compatible.

Edit: but all illumos distros (SmartOS, OmniOS, OpenIndiana, ...) should be though.

Copy link
Member

Choose a reason for hiding this comment

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

Different platform API could hypothetically be shimmed. I'll defer to @janvorli on the feasibility of doing something like portable-linux here and if we need to think about that from the start. I suppose it could always be added later.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it makes sense to create a portable thing here - we don't have portability between MUSL and GLIBC based distros for similar reasons.

Copy link
Member

Choose a reason for hiding this comment

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

I am referring to illumos vs solaris.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also note that illumos to solaris is not only like glibc to musl difference (which is two libcs for same Linux kernel) but they are also freebsd to macOS different (which is two different kernels and their corresponding libcs).

src/libraries/OSGroups.json Outdated Show resolved Hide resolved
eng/Configurations.props Outdated Show resolved Hide resolved
eng/build.sh Outdated Show resolved Hide resolved
eng/build.sh Outdated Show resolved Hide resolved
return ridOS;
}
#elif defined(__sun)
pal::string_t pal::get_current_os_rid_platform()
Copy link
Member

Choose a reason for hiding this comment

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

If I remember correctly, there was a time where we were trying very hard to avoid having special case mapping in the RID calculation. It means folks will need a new copy of the RID algorithm in order to get the right result on these platforms. It will also mean the old algorithm will stop returning the truth on these platforms. @eerhardt

Copy link
Member

Choose a reason for hiding this comment

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

The way the new 5.0 API RuntimeInformation.RuntimeIdentifier is implemented is it just returns the RID from the host. So as long as any users are using this new API, they will get the correct RID.

If users are using the deprecated Microsoft.DotNet.PlatformAbstractions library, they won't get the correct RID on these systems, and will need to update to the new API.

Copy link
Member

Choose a reason for hiding this comment

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

cc @vitek-karas @swaroop-sridhar - for the host changes.

Copy link
Member Author

@am11 am11 Jun 2, 2020

Choose a reason for hiding this comment

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

@eerhardt, does the deprecated PlatformAbstractions library provide certain feature which RuntimeIndentifier does not? I found that dotnet/sdk still using it in the following three files:

sdk$ git grep -li PlatformAbstractions
src/Cli/Microsoft.DotNet.Cli.Utils/WindowsRegistryEnvironmentPathEditor.cs
src/Layout/redist/targets/GenerateLayout.targets
src/Tasks/Microsoft.NET.Build.Tasks/ProjectContext.cs

Generally, I think it would be a better model, if we don't have to update platforms in SDK repo separately each time a new platform is introduced in runtime repo. For example, roslyn, aspnet, even msbuild and many other repos don't know/care how many OS platforms are supported by runtime, they just work everywhere (MSIL ftw). At least SDK should not block the new platforms by default (especially if that hardcoded platform list is just used for telemetry). :)

Copy link
Member

Choose a reason for hiding this comment

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

I found that dotnet/sdk still using it in the following three files:

In the two .cs files, it is only being used by a using statement. This was a left-over of old code, and can be removed. The reason the namespace still exists is because another library (Microsoft.Extensions.DependencyModel) has an Obsolete type in that namespace.

For src/Layout/redist/targets/GenerateLayout.targets, this was actually telling the build system to not include the PlatformAbstractions assembly coming from vstest. vstest doesn't reference that library anymore, so it can be removed as well.

I've removed these with dotnet/sdk#11857.

Generally, I think it would be a better model, if we don't have to update platforms in SDK repo separately each time a new platform is introduced in runtime repo.

At least SDK should not block the new platforms by default (especially if that hardcoded platform list is just used for telemetry). :)

Can you open an issue in dotnet/sdk for this? Note that it isn't just telemetry - but also the SDK prints OS platform information in dotnet --info.

@am11
Copy link
Member Author

am11 commented May 28, 2020

Mono Product Build Linux arm64 debug is unrelated (git clone failed on that machine).

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM. (leaving RID parts to the experts)

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.

I'm OK with mechanical side of the RID changes. I'd like explicit signoff from @eerhardt and @janvorli as well.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

From a RID stand-point this looks good to me.

@am11
Copy link
Member Author

am11 commented Jun 9, 2020

Resolved a merge conflict.
@janvorli, @jkotas, could you please take a look?

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@am11
Copy link
Member Author

am11 commented Jun 11, 2020

All checks have passed
Good to merge? :)

@janvorli janvorli merged commit c783b1f into dotnet:master Jun 11, 2020
@am11 am11 deleted the feature/sunos/rid branch June 11, 2020 13:44
@ericstj
Copy link
Member

ericstj commented Jun 11, 2020

Thank you for contribution @am11

@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
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.

9 participants