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

Add version information to RuntimeInformation.FrameworkDescription. #5210

Merged
merged 1 commit into from
Jan 7, 2016

Conversation

Priya91
Copy link
Contributor

@Priya91 Priya91 commented Jan 6, 2016

No description provided.

@@ -19,7 +20,7 @@ public static string FrameworkDescription
{
get
{
return FrameworkName;
return string.Format("{0} {1}", FrameworkName, typeof(object).GetTypeInfo().Assembly.GetName().Version.ToString());
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think you don't technically need the ToString() call because string.Format will do it for you.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, should we cache that version? Probably doesn't make a whole lot of difference, but I don't think there's any way it could change during runtime

Copy link
Member

Choose a reason for hiding this comment

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

If we're going to cache something, presumably it could be the whole FrameworkDescription result.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here there is no call to OS apis, and hence didnt cache, as this operation is relatively simple. I will cache the whole thing as well, although for future coding practices, how do you decide when to cache and when not to? the lifetime of the string is going to be more instead of when it was a local. So the tradeoff will be between execution time and memory. But here both are insignificant.

Copy link
Member

Choose a reason for hiding this comment

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

for future coding practices, how do you decide when to cache and when not to?

Based on expected / measured usage patterns. If you expect this method to only be invoked rarely, then caching doesn't make sense: the string object(s) will be allocated once and the work to compute those strings done once, and then there's no need to hold onto them and take up space in the process after that. If you instead expect this method will be used repeatedly, then it makes more sense to cache rather than doing the work and allocating each time. And if you expect it to be used really frequently, on a hot path, with every operation mattering, you might even choose to initialize the static as part of its declaration rather than lazily in the property so as to avoid branching costs to check for whether it's been initialized.

For this specific case, it definitely won't fall into the last case. And I doubt it even falls into the middle case. More than likely, caching here is unnecessary. I was simply commenting that if something made sense to cache, it would be the result of the whole operation rather than just the version used in the operation.

@stephentoub
Copy link
Member

LGTM

@Priya91
Copy link
Contributor Author

Priya91 commented Jan 7, 2016

@dotnet-bot Test Innerloop Ubuntu Release Build and Test

@@ -19,7 +20,7 @@ public static string FrameworkDescription
{
get
{
return FrameworkName;
return string.Format("{0} {1}", FrameworkName, typeof(object).GetTypeInfo().Assembly.GetName().Version);
Copy link
Member

Choose a reason for hiding this comment

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

Does "Version" always have the build version number in it? I thought that is what the "FileVersion" attribute was for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Version" holds the build version. Here, am getting the version of the dll holding object, so it should have a value.

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 have the absolute latest code on my Linux machine, but when I run:

    static void Main(string[] args)
    {
        Console.WriteLine(typeof(object).GetTypeInfo().Assembly.GetName().Version);
    }

I get the output:

4.0.0.0

This seems less useful to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The FileVersionInfo apis to get the FileVersion are not available on .NET Native. This is the best I could find, that will work for all platforms. Let me know if I am missing any other APIs that can be used here.

Copy link
Member

Choose a reason for hiding this comment

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

Can't you get the FileVersionAttribute from the assembly using Reflection APIs? And then use it to get the version string.

If that isn't possible, I wouldn't know off the top of my head. @stephentoub or @weshaggard may have a better idea.

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe use a #if and just do the current code for .Net Native, but use FileVersionInfo APIs for everything else.

Copy link
Member

Choose a reason for hiding this comment

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

I know that I might have said to do this and it is better then before I do have to agree with @eerhardt that it isn't all that helpful because it will always be 4.0.0.0. At this point however I also agree with @Priya91 that I don't know if any good way to get a better version, in the context of .NET Native at least because reflection isn't going to work.

@ericstj any other options?

Copy link
Member

Choose a reason for hiding this comment

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

How about a C# code file that is generated by the build with something like:

const string BuildNumber = "< generated Build Number >";

And that's what gets used for this value on .Net Native.

When on official builds < generated Build Number > gets replaced with the official build number. When on any other build we put a "sentinel" value like 0.1 or 0.99 etc to mean it is a private/non-official build. (Or use whatever we use for privately built package numbers now).

Copy link
Member

Choose a reason for hiding this comment

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

@weshaggard we can enable file version info using the UNIX impl on .netnative, though it still feels a bit heavy.

That still wouldn't be correct on .net native since the file with object in it would be the app. Maybe NETNative just transforms the IL to burn in the number.

Burning in a number during the build of this assembly would work some of the time. It's at least a min version, but if someone upgraded just this package they'd get a newer version.

Priya91 added a commit that referenced this pull request Jan 7, 2016
Add version information to RuntimeInformation.FrameworkDescription.
@Priya91 Priya91 merged commit 4b20777 into dotnet:master Jan 7, 2016
@Priya91 Priya91 deleted the addframeworkversion branch January 28, 2016 18:57
@karelz karelz modified the milestone: 1.0.0-rtm Dec 3, 2016
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.

9 participants