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

Add EnvironmentAugments to coreclr #6205

Merged
stephentoub merged 2 commits into from
Jul 13, 2016
Merged

Add EnvironmentAugments to coreclr #6205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
18ad6e5
Add EnvironmentAugments to coreclr
stephentoub Jul 9, 2016
340f1b0
Conditionally compile EnvironmentAugments.cs for coreclr
stephentoub Jul 13, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/mscorlib/model.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1847,6 +1847,22 @@
<Member Name="FailFast(System.String,System.Exception)" />
<Member Name="Exit(System.Int32)" />
</Type>
<Type Name="Internal.Runtime.Augments.EnvironmentAugments">
Copy link
Copy Markdown
Member

@danmoseley danmoseley Jul 9, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

can <Type Name="System.Environment"> come out now then?

Copy link
Copy Markdown
Member Author

@stephentoub stephentoub Jul 9, 2016

Choose a reason for hiding this comment

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

No. If nothing else, corefx master still depends on it. But even after that's changed, I'm not sure we can remove the existing Environment exports, in case we need to support an existing System.Runtime.Extensions.dll working with a newer System.Private.Corelib.dll... not sure if that's a supported scenario or not.

Copy link
Copy Markdown
Member

@danmoseley danmoseley Jul 9, 2016

Choose a reason for hiding this comment

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

Not sure whether we consider S.P.Corelib.dll implicitly undocumented from now on -- whether we retain freedom to remove things after we ship it next ..

Copy link
Copy Markdown
Member Author

@stephentoub stephentoub Jul 9, 2016

Choose a reason for hiding this comment

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

Even if we say we don't support code outside of coreclr/corefx using its surface area (which is a very reasonable thing to say IMO), corefx 1.0 is using this surface area, so the question I was posing was about whether we support using, for example, a 1.0 corefx (and in this specific case a 1.0 System.Runtime.Extensions.dll) with a 1.1 coreclr.

Copy link
Copy Markdown
Member

@jkotas jkotas Jul 9, 2016

Choose a reason for hiding this comment

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

I agree that it needs the Environment need stay for now until corefx is updated. Everything will break otherwise.

I'm not sure we can remove the existing Environment exports, in case we need to support an existing System.Runtime.Extensions.dll working with a newer System.Private.Corelib.dll... not sure if that's a supported scenario or not.

I have said number of times that we need to be a plan where only latest CoreCLR is guaranteed to work with latest System.Runtime.Extensions and friends (assemblies that depend on internal contracts). The matrix of different combinations will go through the roof otherwise, and we are unlikely to ensure that all combinations work anyway.

Copy link
Copy Markdown
Member

@jkotas jkotas Jul 9, 2016

Choose a reason for hiding this comment

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

we support using, for example, a 1.0 corefx (and in this specific case a 1.0 System.Runtime.Extensions.dll) with a 1.1 coreclr

1.0 corefx assemblies that depend on public contracts only should be supported with a 1.1 CoreCLR. For example, System.Reflection.Metadata.

Copy link
Copy Markdown
Member Author

@stephentoub stephentoub Jul 9, 2016

Choose a reason for hiding this comment

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

I have said number of times that we need to be a plan where only latest CoreCLR is guaranteed to work with latest System.Runtime.Extensions and friends

I'm happy with that answer. 😄

<Member MemberType="Property" Name="CurrentManagedThreadId" />
<Member MemberType="Property" Name="ExitCode" />
<Member MemberType="Property" Name="HasShutdownStarted" />
<Member MemberType="Property" Name="StackTrace" />
<Member MemberType="Property" Name="TickCount" />
<Member Name="get_CurrentManagedThreadId" />
<Member Name="get_ExitCode" />
<Member Name="set_ExitCode(System.Int32)" />
<Member Name="get_HasShutdownStarted" />
<Member Name="get_StackTrace" />
<Member Name="get_TickCount" />
<Member Name="Exit(System.Int32)" />
<Member Name="FailFast(System.String,System.Exception)" />
<Member Name="GetCommandLineArgs" />
</Type>
<Type Name="System.EventArgs">
<Member MemberType="Field" Name="Empty" />
<Member Name="#ctor" />
Expand Down
3 changes: 3 additions & 0 deletions src/mscorlib/mscorlib.shared.sources.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -450,6 +450,9 @@
<SystemSources Condition="'$(FeatureCominterop)' == 'true'" Include="$(BclSourcesRoot)\System\Variant.cs" />
<SystemSources Condition="'$(FeatureClassicCominterop)' == 'true'" Include="$(BclSourcesRoot)\System\OleAutBinder.cs" />
</ItemGroup>
<ItemGroup>
<SystemSources Condition="'$(FeatureCoreclr)' == 'true'" Include="$(BclSourcesRoot)\Internal\Runtime\Augments\EnvironmentAugments.cs" />
</ItemGroup>
<ItemGroup>
<ReflectionSources Include="$(BclSourcesRoot)\System\Reflection\__Filters.cs" />
<ReflectionSources Include="$(BclSourcesRoot)\System\Reflection\AmbiguousMatchException.cs" />
Expand Down
21 changes: 21 additions & 0 deletions src/mscorlib/src/Internal/Runtime/Augments/EnvironmentAugments.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;

namespace Internal.Runtime.Augments
{
/// <summary>For internal use only. Exposes runtime functionality to the Environments implementation in corefx.</summary>
public static class EnvironmentAugments
{
public static int CurrentManagedThreadId => Environment.CurrentManagedThreadId;
public static void Exit(int exitCode) => Environment.Exit(exitCode);
public static int ExitCode { get { return Environment.ExitCode; } set { Environment.ExitCode = value; } }
Copy link
Copy Markdown
Member

@jkotas jkotas Jul 9, 2016

Choose a reason for hiding this comment

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

I do not think that the ExitCode setter works as expected in CoreCLR today.

Copy link
Copy Markdown
Member Author

@stephentoub stephentoub Jul 9, 2016

Choose a reason for hiding this comment

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

I'll add a test for it in corefx. If it doesn't work I'll open an issue on that. I don't think we'd change this augments implementation based on that, but rather would fix in the runtime whatever we'd need to get it working. There's already a corefx test verifying that this value roundtrips, but that doesn't mean it actually ends up propagating the value out of the process.

Copy link
Copy Markdown
Member

@jkotas jkotas Jul 9, 2016

Choose a reason for hiding this comment

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

Agree - it would need to be fixed in runtime. Same goes for CoreRT - we should make it work in .NET Native / CoreRT as well.

Copy link
Copy Markdown
Member Author

@stephentoub stephentoub Jul 9, 2016

Choose a reason for hiding this comment

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

You're correct; I just verified it doesn't propagate to the process' exit code.

public static void FailFast(string message, Exception error) => Environment.FailFast(message, error);
public static string[] GetCommandLineArgs() => Environment.GetCommandLineArgs();
public static bool HasShutdownStarted => Environment.HasShutdownStarted;
public static string StackTrace => Environment.StackTrace;
Copy link
Copy Markdown
Member

@jkotas jkotas Jul 10, 2016

Choose a reason for hiding this comment

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

This will return extra useless lines in the stacktrace based on how the optimizations kicked in. We will need to:

  • Disable optimizations on the forwarder in corefx to make sure that there is fixed number of frames to skip
  • Skip the given number of frames here by calling StackTrace constructor with skipFrames argument, instead of just forwarding to Environment.

Copy link
Copy Markdown
Member

@jkotas jkotas Jul 10, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

(Similar for corert.)

Copy link
Copy Markdown
Member Author

@stephentoub stephentoub Jul 10, 2016

Choose a reason for hiding this comment

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

Opened https://github.com/dotnet/coreclr/issues/6209 to track

public static int TickCount => Environment.TickCount;
}
}