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

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Feb 17, 2019

@jkotas
Copy link
Member Author

jkotas commented Feb 17, 2019

cc @richlander

{
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

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.

@davidfowl
Copy link
Member

davidfowl commented Feb 17, 2019

This won't work for pre-releases, it'll return an empty or incomplete version. This is the reason why I stopped pushing on this for to be returned from Environment.Version even though what we return today is pretty useless so maybe this is better than nothing. Is it worth having another property on environment that is a string ProductVersion or something? We need to do this https://github.com/dotnet/corefx/issues/13526 😄

@jkotas
Copy link
Member Author

jkotas commented Feb 17, 2019

on environment that is a string ProductVersion or something?

We do have RuntimeInformation.FrameworkDescription that returns string. It contains bogus version today as well.

I think we should first fix existing APIs to return sensible versions. If that proves to be insufficient, we can add a yet another version string returning API.

@davidfowl
Copy link
Member

Sure we can, and people can always get FX_PRODUCT_VERSION directly as a workaround. It's just that it'll show slightly less accurate information but it's better than what we have today.

jkotas added a commit to dotnet/corefx that referenced this pull request Feb 21, 2019
* Omit Environment.Version from CodeDom generated files

Contributes to dotnet/coreclr#22664

* Disable tests on .NET Framework
@jkotas jkotas merged commit 83e9a39 into dotnet:master Feb 23, 2019
Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/corefx that referenced this pull request Feb 23, 2019
…22664)

* Change Environment.Version to return product version

- Contributes to https://github.com/dotnet/corefx/issues/31099
- Use AssemblyInformationalVersion attribute as fallback

* Add sanity test for Environment.Version

* Disable CodeDom tests

* Fix test assembly name

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/corert that referenced this pull request Feb 23, 2019
…22664)

* Change Environment.Version to return product version

- Contributes to https://github.com/dotnet/corefx/issues/31099
- Use AssemblyInformationalVersion attribute as fallback

* Add sanity test for Environment.Version

* Disable CodeDom tests

* Fix test assembly name

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/mono that referenced this pull request Feb 23, 2019
…22664)

* Change Environment.Version to return product version

- Contributes to https://github.com/dotnet/corefx/issues/31099
- Use AssemblyInformationalVersion attribute as fallback

* Add sanity test for Environment.Version

* Disable CodeDom tests

* Fix test assembly name

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
jkotas added a commit to dotnet/corefx that referenced this pull request Feb 23, 2019
…22664)

* Change Environment.Version to return product version

- Contributes to https://github.com/dotnet/corefx/issues/31099
- Use AssemblyInformationalVersion attribute as fallback

* Add sanity test for Environment.Version

* Disable CodeDom tests

* Fix test assembly name

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
@jkotas jkotas deleted the environment-version branch February 23, 2019 07:29
marek-safar pushed a commit to mono/mono that referenced this pull request Feb 23, 2019
…22664)

* Change Environment.Version to return product version

- Contributes to https://github.com/dotnet/corefx/issues/31099
- Use AssemblyInformationalVersion attribute as fallback

* Add sanity test for Environment.Version

* Disable CodeDom tests

* Fix test assembly name

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
jkotas added a commit to dotnet/corert that referenced this pull request Feb 25, 2019
…22664)

* Change Environment.Version to return product version

- Contributes to https://github.com/dotnet/corefx/issues/31099
- Use AssemblyInformationalVersion attribute as fallback

* Add sanity test for Environment.Version

* Disable CodeDom tests

* Fix test assembly name

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
…35458)

* Omit Environment.Version from CodeDom generated files

Contributes to dotnet/coreclr#22664

* Disable tests on .NET Framework


Commit migrated from dotnet/corefx@89f72f6
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
…22664)

* Change Environment.Version to return product version

- Contributes to https://github.com/dotnet/corefx/issues/31099
- Use AssemblyInformationalVersion attribute as fallback

* Add sanity test for Environment.Version

* Disable CodeDom tests

* Fix test assembly name


Commit migrated from dotnet/coreclr@83e9a39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants