Add EnvironmentAugments to coreclr by stephentoub · Pull Request #6205 · dotnet/coreclr

stephentoub · 2016-07-09T21:50:13Z

Continuation of dotnet/corefx#9851
cc: @jkotas, @danmosemsft

danmoseley · 2016-07-09T22:02:15Z

      <Member Name="FailFast(System.String,System.Exception)"  />
      <Member Name="Exit(System.Int32)" />
    </Type>
+    <Type Name="Internal.Runtime.Augments.EnvironmentAugments">


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

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.

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 ..

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.

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.

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.

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. 😄

danmoseley · 2016-07-09T22:02:36Z

LGTM

jkotas · 2016-07-09T22:08:38Z

+    {
+        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; } }


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

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.

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

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

jkotas · 2016-07-09T22:31:45Z

LGTM

jkotas · 2016-07-10T14:16:50Z

+        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;


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.

(Similar for corert.)

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

ellismg · 2016-07-13T01:23:13Z

This is going to cause build breaks on the TFS side, perhaps, because of the model.xml updates. I will coordinate the merge with some other changes that are touching model.xml and fix up the build breaks.

@dotnet-bot test Linux ARM Emulator Cross Debug Build please
@dotnet-bot test Linux ARM Emulator Cross Release Build please

(Both of the above failures look infrastructure related, a device could not unmount).

jkotas · 2016-07-13T03:58:15Z

    <SystemSources Condition="'$(FeatureCominterop)' == 'true'" Include="$(BclSourcesRoot)\System\Variant.cs" />
    <SystemSources Condition="'$(FeatureClassicCominterop)' == 'true'" Include="$(BclSourcesRoot)\System\OleAutBinder.cs" />
  </ItemGroup>
+  <ItemGroup>


This should have Condition="'$(FeatureCoreClr)'=='true'"

jkotas · 2016-07-13T04:00:13Z

This is going to cause build breaks on the TFS side

The asmmeta updates are no longer needed for CoreCLR, they are needed for full framework only. As long as this is changing just CoreCLR - we should make this happen by including the new file for CoreCLR only, it will be fine.

stephentoub · 2016-07-13T20:52:22Z

As long as this is changing just CoreCLR - we should make this happen by including the new file for CoreCLR only, it will be fine.

Added the condition to the entry.

…gments Add EnvironmentAugments to coreclr Commit migrated from dotnet/coreclr@c93479b

dnfclas added the cla-already-signed label Jul 9, 2016

stephentoub force-pushed the environmentaugments branch from 50ff539 to 1048832 Compare July 9, 2016 21:51

danmoseley reviewed Jul 9, 2016
View reviewed changes

jkotas reviewed Jul 9, 2016
View reviewed changes

jkotas mentioned this pull request Jul 9, 2016

Add EnvironmentAugments to corert dotnet/corert#1496

Merged

jkotas reviewed Jul 10, 2016
View reviewed changes

ellismg mentioned this pull request Jul 13, 2016

Add back some surface area to thinned corelib #6247

Merged

jkotas reviewed Jul 13, 2016
View reviewed changes

stephentoub added 2 commits July 13, 2016 16:49

Add EnvironmentAugments to coreclr

18ad6e5

Conditionally compile EnvironmentAugments.cs for coreclr

340f1b0

stephentoub force-pushed the environmentaugments branch from 1048832 to 340f1b0 Compare July 13, 2016 20:52

stephentoub merged commit c93479b into dotnet:master Jul 13, 2016

stephentoub deleted the environmentaugments branch July 13, 2016 22:51

jamesqo mentioned this pull request Aug 1, 2016

Slightly increase throughput of string.Concat(object[]) #6547

Merged

stephentoub mentioned this pull request Jan 31, 2020

Environment.StackTrace includes unnecessary internal calls dotnet/runtime#6294

Closed

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/coreclr#6205 from stephentoub/environmentau…

0a3152e

…gments Add EnvironmentAugments to coreclr Commit migrated from dotnet/coreclr@c93479b

riQQ mentioned this pull request Jan 19, 2023

Expose remaining Environment.* public surface area from System.Private.Corelib #6006

Closed

Conversation

stephentoub commented Jul 9, 2016

Uh oh!

danmoseley Jul 9, 2016 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmoseley commented Jul 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Jul 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Jul 10, 2016 • 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!

Choose a reason for hiding this comment

Uh oh!

ellismg commented Jul 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Jul 13, 2016

Uh oh!

stephentoub commented Jul 13, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

danmoseley Jul 9, 2016 •

edited

Loading

jkotas Jul 10, 2016 •

edited

Loading