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

Solve problems #1442 #1702

Closed
wants to merge 1 commit into from
Closed

Conversation

SouJack
Copy link

@SouJack SouJack commented Mar 10, 2021

A small change to remove the dependency on the "System.Diagnostics.PerformanceCounter" package for non-Windows binary projects.
For #1442

@mgravell
Copy link
Collaborator

With the definition

<DefineConstants Condition="'$(TargetFramework)' != 'net461' and '$(TargetFramework)' != 'net472'">$(DefineConstants);XAMARIN</DefineConstants>

This isn't really "am I Xamarin?", but is instead: "am I anything except .NET Framework", and since .NET Framework is only really retained for legacy convenience, it is really "am I anything?"

I would prefer to be much more selective in this issue, if possible - perhaps checking the runtime environment to isolate places where we know it will fail, rather than just assuming "not .NET Framework? it'll fail". Perhaps via one of the RuntimeInformation APIs?

@SouJack
Copy link
Author

SouJack commented Mar 10, 2021

In fact, it is not really xamarin. But it has been tested and is being used at runtime. The solution can be more elaborate, I agree, but, fact that the solution for those who want to use in XF works correctly both Android and iOS. And of course it still works normally in .Net Core since the Nuget package itself does not contain the separation for xamarin runtimes.
It is a suggested course to follow for a final solution.

@mgravell
Copy link
Collaborator

Your suggested "fix" is to remove functionality that was intentionally added, from all non-obsolete builds. While that is an option, it is one that needs a little more consideration. What are we losing? Is it still needed/wanted? How much does it concern us? Etc.

@SouJack
Copy link
Author

SouJack commented Mar 10, 2021

That's correct, but by your own code and comments, if you can't get access to the object, you return false, what I did was remove it, since for Android and iOS the package doesn't exist. As I said earlier, this is only a suggestion following the logic of writing in the class in question.
For me it is working perfectly and I see that there are people with the same problem, until a definitive suggestion comes out I think it can be useful.

@mgravell
Copy link
Collaborator

mgravell commented Mar 10, 2021 via email

@SouJack SouJack closed this Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants