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

Change Environment.Version to return product version #22664

Merged
merged 5 commits into from
Feb 23, 2019
Merged

Change Environment.Version to return product version #22664

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
183d60b
Change Environment.Version to return product version
jkotas Feb 17, 2019
e1327da
Add sanity test for Environment.Version
jkotas Feb 20, 2019
750c0c5
Disable CodeDom tests
jkotas Feb 20, 2019
fdfb2ce
Fix test assembly name
jkotas Feb 22, 2019
df31d3f
Merge branch 'master' into environment-version
jkotas Feb 22, 2019
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
13 changes: 13 additions & 0 deletions src/System.Private.CoreLib/System.Private.CoreLib.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,19 @@
<SkipCommonResourcesIncludes>true</SkipCommonResourcesIncludes>
<DocumentationFile>$(OutputPath)$(MSBuildProjectName).xml</DocumentationFile>
</PropertyGroup>

<!-- Approximate Arcade version string https://github.com/dotnet/arcade/blob/master/Documentation/CorePackages/Versioning.md -->
<!-- This workaround should be removed once CoreCLR repo is updated to use Arcade -->
<PropertyGroup>
<InformationalVersion>$(VersionPrefix)</InformationalVersion>
</PropertyGroup>
<PropertyGroup Condition="'$(PreReleaseLabel)' != ''">
<InformationalVersion>$(InformationalVersion)-$(PreReleaseLabel).$(BuildNumberMajor).$(BuildNumberMinor)</InformationalVersion>
</PropertyGroup>
<PropertyGroup Condition="'$(LatestCommit)' != ''">
<InformationalVersion>$(InformationalVersion)+$(LatestCommit)</InformationalVersion>
</PropertyGroup>

<!-- Add Serviceable attribute to the project's metadata -->
<ItemGroup>
<AssemblyMetadata Include="Serviceable">
Expand Down
28 changes: 24 additions & 4 deletions src/System.Private.CoreLib/shared/System/Environment.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,10 +129,30 @@ public static OperatingSystem OSVersion

public static bool UserInteractive => true;

// Previously this represented the File version of mscorlib.dll. Many other libraries in the framework and outside took dependencies on the first three parts of this version
// remaining constant throughout 4.x. From 4.0 to 4.5.2 this was fine since the file version only incremented the last part. Starting with 4.6 we switched to a file versioning
// scheme that matched the product version. In order to preserve compatibility with existing libraries, this needs to be hard-coded.
public static Version Version => new Version(4, 0, 30319, 42000);
public static Version Version
{
get
{
// FX_PRODUCT_VERSION is expected to be set by the host
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't appear to be set by corerun, unless I'm just missing it. Can we do that?

Copy link
Member Author

Choose a reason for hiding this comment

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

What should we set it to in corerun?

Copy link
Member

@stephentoub stephentoub Feb 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Whatever we expect it to be set to by dotnet? It otherwise seems strange to me that the same build of coreclr would produce different results for Environment.Version. I guess my concern is that this is configurable in this way at all by the host. Why is that the approach? Maybe I just don't understand what Version is meant to be, but the docs say it's the version of the common language runtime.

Copy link
Member Author

Choose a reason for hiding this comment

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

We do not have product version (ie the marketing version) available when building CoreCLR repo: https://github.com/dotnet/corefx/issues/31099#issuecomment-407190291

I guess we can redo how the build works to make it available somehow, but I have no idea what it would take.

Copy link
Member

Choose a reason for hiding this comment

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

I guess we can redo how the build works to make it available somehow

This makes the most sense to me, Environment.Version always returning the same version for the same CoreLib.dll build, regardless of who is hosting it, what an appcontext variable was set to, etc. It just doesn't seem like something that should be dynamic in any way.

But I also don't know what it would take. How does dotnet know the marketing version? Is that not configured in its build somehow?

Copy link
Member Author

Choose a reason for hiding this comment

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

Environment.Version always returning the same version for the same CoreLib.dll build

+1

How does dotnet know the marketing version?

It reads it from *.deps.json file (either from shared framework, or from the one in self-contained app).

I also don't know what it would take.

It seems to be available here: https://github.com/dotnet/coreclr/blob/master/eng/Versions.props#L13 so we just need to embed this property in the CoreLib.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mmitche Could you please confirm whether MicrosoftNETCoreAppVersion property is set to the current product version during the official builds?

Copy link
Member

Choose a reason for hiding this comment

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

It wouldn't be (neither for 2.1 or 2.2), The version is flowed from the last build of core-setup that was done. Since this is a circular dependency, there's no to have pre-knowledge of what the netcore app version will be.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I think the best compromise is to use FX_PRODUCT_VERSION when it is set, and use approximate version used to build CoreCLR as fallback (https://github.com/dotnet/coreclr/blob/master/eng/Versions.props#L5).

We use similar logic for AppContext.BaseDirectory: https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/shared/System/AppContext.cs#L26

Copy link
Member

Choose a reason for hiding this comment

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

the best compromise

Sounds good.

string versionString = (string)AppContext.GetData("FX_PRODUCT_VERSION");

if (versionString == null)
{
// Use AssemblyInformationalVersionAttribute as fallback if the exact product version is not specified by the host
versionString = typeof(object).Assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>()?.InformationalVersion;
}

ReadOnlySpan<char> versionSpan = versionString.AsSpan();

// Strip optional suffixes
int separatorIndex = versionSpan.IndexOfAny("-+ ");
if (separatorIndex != -1)
versionSpan = versionSpan.Slice(0, separatorIndex);

// Return zeros rather then failing if the version string fails to parse
return Version.TryParse(versionSpan, out Version version) ? version : new Version();
}
}

public static long WorkingSet
{
Expand Down
38 changes: 38 additions & 0 deletions tests/CoreFX/CoreFX.issues.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -883,6 +883,10 @@
"name": "System.Tests.MathFTests.Min",
"reason": "https://github.com/dotnet/coreclr/pull/20912"
},
{
"name": "System.Tests.EnvironmentTests.Version_MatchesFixedVersion",
"reason": "outdated"
},
]
}
},
Expand Down Expand Up @@ -1557,4 +1561,38 @@
]
}
},
{
"name": "System.CodeDom.Tests",
"enabled": true,
"exclusions": {
"namespaces": null,
"classes": null,
"methods": [
{
"name": "System.CodeDom.Compiler.Tests.CSharpCodeGenerationTests.ProviderSupports",
"reason": "outdated"
},
{
"name": "System.CodeDom.Compiler.Tests.CSharpCodeGenerationTests.MetadataAttributes",
"reason": "outdated"
},
{
"name": "System.CodeDom.Compiler.Tests.CSharpCodeGenerationTests.RegionsSnippetsAndLinePragmas",
"reason": "outdated"
},
{
"name": "System.CodeDom.Compiler.Tests.VBCodeGenerationTests.RegionsSnippetsAndLinePragmas",
"reason": "outdated"
},
{
"name": "System.CodeDom.Compiler.Tests.VBCodeGenerationTests.MetadataAttributes",
"reason": "outdated"
},
{
"name": "System.CodeDom.Compiler.Tests.VBCodeGenerationTests.ProviderSupports",
"reason": "outdated"
}
]
}
}
]
38 changes: 38 additions & 0 deletions tests/src/CoreMangLib/system/environment/environment_version.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// 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;

class enviroment_version
{
static int Main()
{
Version ver = Environment.Version;
Console.WriteLine($"Environment.Version = {ver}");

if (ver < new Version("3.0"))
{
Console.WriteLine("ERROR: Version less than 3.0.");
return -1;
}

// Verify that we are not returning hardcoded version from .NET Framework.
if (ver == new Version("4.0.30319.42000"))
{
Console.WriteLine("ERROR: Version is hardcoded .NET Framework version.");
return -1;
}

// .NET Core assemblies use 4.6+ as file version. Verify that we have not used
// the file version as product version by accident.
if (ver.Major == 4 && (ver.Minor >= 6))
{
Console.WriteLine("ERROR: Version is 4.6+.");
return -1;
}

Console.WriteLine("PASSED");
return 100;
}
}
36 changes: 36 additions & 0 deletions tests/src/CoreMangLib/system/environment/environment_version.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
<?xml version="1.0" encoding="utf-8"?>
<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
<PropertyGroup>
<Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
<Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
<SchemaVersion>2.0</SchemaVersion>
<ProjectGuid>{95DFC527-4DC1-495E-97D7-E94EE1F7140D}</ProjectGuid>
<OutputType>Exe</OutputType>
<ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
<SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<CLRTestKind>BuildAndRun</CLRTestKind>
<CLRTestPriority>0</CLRTestPriority>
</PropertyGroup>
<!-- Default configurations to help VS understand the configurations -->
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
</PropertyGroup>
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' ">
</PropertyGroup>
<ItemGroup>
<CodeAnalysisDependentAssemblyPaths Condition=" '$(VS100COMNTOOLS)' != '' " Include="$(VS100COMNTOOLS)..\IDE\PrivateAssemblies">
<Visible>False</Visible>
</CodeAnalysisDependentAssemblyPaths>
</ItemGroup>
<ItemGroup>
<!-- Add Compile Object Here -->
<Compile Include="environment_version.cs" />
</ItemGroup>
<ItemGroup>
<Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
</ItemGroup>
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
<PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' ">
</PropertyGroup>
</Project>