-
Notifications
You must be signed in to change notification settings - Fork 154
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
Find best version of Mono under Windows #45
Conversation
@rprouse I hope you have not been holding off on this because of the "do not merge" comment. You're the primary person with whom the direction of the fix needs to be resolved! |
@nunit/core-team This has been waiting for comments/review for more than two weeks. It's a high priority bug so I'd really like to finish it. |
@CharliePoole, sorry, missed this. I guess that is one of the problems with multiple repositories, too many places to look. I wonder if we should experiment with assigning PR's to one or more team members so they appear in the Pull Requests menu at the top of the page instead of having to drill into each repository? Back to the issue at hand. I don't use Mono except on Linux and even there I am switching to .NET Core, so I don't have much experience with the Windows experience. Correct me if I am wrong, but I believe that to use mono on Windows, you need to launch using As I see it, we only offer the option to use specific .NET Frameworks on Windows because the user has no other way of specifying it. Mono is different because you must launch using the mono executable. So, should we not just detect the version of Mono that we are running under and launch the agent using the same? Then for the various platform attributes, we just need to compare to the running version. What are the holes in my thinking? I'm sure I'm missing something here 😄 |
Your logic is great, but your initial premise has led you astray. :-) We have always supported running either the gui or the console runner under Windows but executing the tests under Mono. For the console, we did this via the /framework option. The gui has a selection menu, which lists mono if it is found. In fact, it's even possible to run under mono and execute tests using Windows - a wholly unnecessary feature that I implemented! However, NUnit V2 only found and listed a single version of mono and the logic we inherited in V3 did the same. Because of changes in how mono installs on windows, we only detect versions older than 3.12, which seems pretty unacceptable. I wrote issue #34 to cope with that problem. In working on it, I ran into a few issues:
OPTIONS We could use the current logic to list all the versions possible and add an internal package setting to pick the version to be used. It makes no sense to me for the engine to list multiple installations of mono and not allow one of them to be selected. If we did this, it would be up to each runner to decide whether to offer the choice of version to the user, but I would favor taking advantage of the capability for the console and gui runners if it's there. We could use the same logic I have implemented, but only return the best (probably latest) version of mono that we find. That would make multiple installations of mono unusable on Windows from NUnit's point of view. As a modification of the last point, when running under Mono, we could use that version rather than the latest for running tests. That would allow someone to test under an older mono version if desired. Basically, I think I need to either take this PR farther or take it back a step! |
Personally, this is my preference. My thinking is that running your code in production using Mono on Windows is a bad idea, the Microsoft framework is much more stable. I see Mono as an option for running your code on other systems (Linux, Mac, Unix, etc). Based on that, you should be running your tests using the framework you are using on the target platform. Running tests using Mono on Windows is useful for developers as a sanity check, but I don't think it is a valid test that they will run on other platforms. Based on this, I don't think we should go out of our way to add support. Feel free to start a flame war over that opinion 😆
Based on my comments above, I don't like this approach because of the amount of work for the gain. It would be the ideal solution, but I think it is one that we will need to continue to tweak going forward and based on your comments, we will not find all installed versions, only latest?
I would be okay with this although it doesn't work with newer versions if Mono isn't installed in the default directory correct?
Isn't this basically the first approach? |
And to add to those comments, my gut feeling is that the use of Mono on Linux and Mac will fade in favour of .NET Core, so we shouldn't go too far out of our way to keep up. I notice that in the list of frameworks in the first comment to this PR that 4.5.1, 4.6, etc are not listed even though they are likely installed. Isn't working on that more important? I would also ask how far we want to go with the whole running under different frameworks now that the release cycle has become so frequent. Can we keep up? |
@rprouse Just to clarify what we do and don't detect, I view mono versions as falling into four groups...
|
@rprouse Re: Detection of higher levels than .NET 4.5 - I agree, we should detect those. Nobody has filed that issue, however. :-) |
@rprouse So for you the preferred option would be...
The two I marked with question marks are items where I'm not sure if that's what you intended. Aside from the fact that this is a lot of "removing" :-) this would be the first time we have used conditional code at runtime based on whether we are running on windows or mono. Currently, we only test the OS but not the runtime and we treat .NET and Mono as equivalent. From your comments, it looks like your second choice is to continue to work the way we do by allowing selection of one default version of mono - but fixing the bug where we don't find versions newer than 3.10. I'd be OK with that as well. We do detect the newest versions of Mono correctly. The mono folks went through a period where they didn't think the registry was needed, but an issue was raised and they started using it again with 4.2. At any rate, I'd prefer this option since it doesn't depend on removing features or working differently on Mono and .NET. We could use the most recent one as the default, working backwards. We could even stop recognizing the older ones if we liked, but I see no particular reason to do that at the moment. My last sub-option wasn't clear... What I meant was that the default mono version would be the one you were running rather than the most recent. So if you managed to run nunit3-console under Mono 3.2.8, the option |
A further comment about the future of this feature... My assumption is that we will eventually (maybe even soon) run tests in other machines than the one on which we are executing or in the cloud. In that case, we may want to have some sort of directory of what runtimes are avalable on each machine and the Mono version, among other things, might come back into play. |
I am fine with that. It sounds like the majority of work is done with this PR. Is there anything else you want to add or should I review and merge? |
I'll have to modify the PR to look for newer versions first and stop at the first one it finds. |
I changed the title from "Find all versions..." to "Find best version..." as a result of discussion on the PR. If the engine is running under mono, it will find and use the version under which it is running. If it is running under Windows, it will find and use the newest installed version of mono, provided it is recorded in the registry or installed in a default location. Except as above, the user can only switch versions of mono by running the NUnit console under a that version. This will be the case under both Windows and Linux. |
039fdd9
to
0ac2044
Compare
@rprouse This is virtually impossible to unit test on a single machine since it works differently depending on what is installed. Some functional tests would help in the future. For now, I've run this in various ways and with various mono versions installed and it has been working for me in all cases. |
Looks good and CI Passed, merging. |
@CharliePoole - this file was added with #45 - I presume it was accidental?
What was happening:
We have not been properly recognizing mono as installed on windows since they stopped using the "Novell" key to store information. That was back around 4.0 or earlier. Additionally, in the past, we only recognized one mono installation even though it was possible to have multiple versions installed.
What's happening now (in this PR):
MonoVersion
if the runtime is mono.What's not addressed:
Although we can list all these frameworks as available, there is no way to select among multiple installed versions of Mono. The arguments we pass in the console and - more important - use internally are simply of the form
mono-2.0
,mono-4.5
, etc. There is no established coding for use of a particular version of mono.This would also apply to the GUI. We could list the versions of mono available and allow the user to select one easily enough. But the GUI would have no way to tell the engine which one to use.
My conclusion is that we have to do one of two things:
If we decide to go with option 1, I would prefer to merge this change and deal with implementing that as a separate issue. If we go with option 2, then I should back out the ability to list multiple options.
Linux Note
On Linux, it's possible to have multiple mono environments, but there's no standard way to find them all as far as I know. Generally, if you are using a parallel environment, you will run the console using that mono. That's the way I've handled Linux in this PR. The only mono it finds is the one under which it's already running.
Discussion
@nunit/contributors @nunit/core-team
Let's discuss and decide what we want our runners to do, bearing in mind that if we want any runner to be able to select among multiple mono versions, then the engine has to support it. Once we have decided, I'll be able to finish up this PR.
Update:
We resolved this in favor of option 2 - just listing a single mono version.