Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eliminate need for CLR version in RuntimeFramework class #1177

Closed
CharliePoole opened this issue Mar 24, 2022 · 3 comments · Fixed by #1179
Closed

Eliminate need for CLR version in RuntimeFramework class #1177

CharliePoole opened this issue Mar 24, 2022 · 3 comments · Fixed by #1179
Assignees
Milestone

Comments

@CharliePoole
Copy link
Collaborator

As @manfred-brands pointed out in #1176, this code makes the engine a bit fragile as new runtimes are introduced.

We could, of course, invent some fake CLR version to return here, but that might cause the problem to pop up elsewhere.

The code in question is only called to populate the ClrVersion property of the RuntimeFramework class. Let's take a look at where that property is used and see if we really need it. If it isn't, let's remove it. Or if it's only needed in our .NET Framework builds, let's make it conditional, which would keep the problem from arising as new runtimes are released.

@manfred-brands I'm assigning this to you as requested. You should note that our longer-term goal is to replace the RuntimeFramework class with something new, based around FrameworkName. So if this particular fix gets too complicated, we should move on to replacement. However, if RuntimeFramework can be simplified first, replacement will then become a bit easier.

@manfred-brands
Copy link
Member

@CharliePoole The method RuntimeFrameworkServices.GetBestAvailableFramework doesn't seem to be called. Can it be deleted?
If so them all but one usage of the ClrVersion property can then be removed.
That only leaves: string monoOptions = "--runtime=v" + TargetRuntime.ClrVersion.ToString(3);

@CharliePoole
Copy link
Collaborator Author

@manfred-brands Yes, if it's not called... if you only verified that using GitHub, double check when you are working in the project as GitHub seems to occasionally miss references.

As for launching under mono with monoOptions, it depends whether you have mono installed in order to test. You could put something inline or just not specify the particular framework at all and see if it will run.

However, I think we may need to do a separate "get mono working" issue once the engine is reorganized as we want it. Mono is much less important than it once was but some people still need it. If you can't do something under mono, I'd favor temporarily commenting out the part of the code, which runs under mono and adding a TODO and maybe a new issue.

Use your judgement as to how much work you want to put into this one, 'cause there's lots more to do. :-)

@manfred-brands
Copy link
Member

I checked that in VS, but as it is a public method on a public class, I didn't know if it was part of some extension API.

Ok, I removed CLRVersion and move the testing logic to the Runtime class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants