pythonnet / clr-loader

Loader for different .NET runtimes
MIT License
32 stars 23 forks source link

Fix to find dotnet root using architecture name #41

Closed LeeDongGeon1996 closed 1 year ago

LeeDongGeon1996 commented 2 years ago
filmor commented 2 years ago

This removes the detection of 32bit binaries, though, is that intended?

LeeDongGeon1996 commented 2 years ago

Actually, it seems that we don't need that part because dotnet does not have any separate installer for 32bits.🤯

DareDevilDenis commented 1 year ago

I have a concern about this. This Pull Request updates the logic to:

    elif sys.platform == "darwin":
        if "ARM" in platform.machine().upper():
            dotnet_root = Path("/usr/local/share/dotnet")
        else:
            dotnet_root = Path("/usr/local/share/dotnet/x64")

However on my Intel based mac, the .NET runtimes are installed to: /usr/local/share/dotnet/ E.g: /usr/local/share/dotnet/host/fxr/6.0.3/libhostfxr.dylib

LeeDongGeon1996 commented 1 year ago

@DareDevilDenis Can you let me know which dotnet version you are using?

DareDevilDenis commented 1 year ago

@LeeDongGeon1996, I'm using .NET runtime 6.0.3.

The following page: https://learn.microsoft.com/en-us/dotnet/core/install/macos says:

On an Arm-based Mac, all Arm64 versions of .NET are installed to the normal /usr/local/share/dotnet/ folder. However, when you install the x64 version of .NET 7 SDK, it's installed to the /usr/local/share/dotnet/x64/dotnet/ folder.

So it looks like the folder is:

LeeDongGeon1996 commented 1 year ago

@DareDevilDenis Sorry, but I don't think so. In my understanding, it's saying that x64 SDK has its own folder named with /dotnet/x64, while ARM SDK is always installed in normal root, just /dotnet

Please have a look at the next section, you mentioned

DareDevilDenis commented 1 year ago

Hi @LeeDongGeon1996. I politely disagree. In https://learn.microsoft.com/en-us/dotnet/core/install/macos, the sections "Path differences" and "Path conflicts" are under the section "Arm-based Macs" where it says "The following sections describe things you should consider when installing .NET on an Arm-based Mac."

So for .NET 6 onwards I still think that my statement above is true:

So it looks like the folder is:

Intel CPU: /usr/local/share/dotnet/ Arm CPU, Arm64 .NET: /usr/local/share/dotnet/ Arm CPU, x64 .NET: /usr/local/share/dotnet/x64/

For .NET prior to version 6, I think the path is always /usr/local/share/dotnet/

I have just tested this on an Intel mac and an Arm mac, and I can confirm that the above paths are correct.

LeeDongGeon1996 commented 1 year ago

@DareDevilDenis Ah, thank you for correcting. I'll change the code, applying the path you figured out.

LeeDongGeon1996 commented 1 year ago

Hi, @DareDevilDenis . I have a question. Do we need to detect x64 SDK on ARM machine? I mean does it work on ARM machine even if it can be installed?

DareDevilDenis commented 1 year ago

Hi @LeeDongGeon1996. Sorry for the delay. I will check this and let you know.

DareDevilDenis commented 1 year ago

Hi @LeeDongGeon1996. I ran the following tests on an Arm based mac, with x64 .NET runtime v6.0.11 (not Arm64):

Both tests worked fine.

Maybe the logic in find_dotnet_root, under 'elif sys.platform == "darwin":' should be:

filmor commented 1 year ago

I'm ready to merge and release whatever you two decide, I just can't test it myself ;)

DareDevilDenis commented 1 year ago

Hi @LeeDongGeon1996, you could use the following:

elif sys.platform == "darwin":
    dotnet_root = Path("/usr/local/share/dotnet")

    # Special case when 'Intel-only' Python is running on an Arm based mac.
    if 'ARM' not in platform.machine().upper():
        result = subprocess.run(['sysctl', '-n', 'machdep.cpu.brand_string'], stdout=subprocess.PIPE, text=True)
        if 'Apple' in result.stdout:
            dotnet_root = Path("/usr/local/share/dotnet/x64")

Running the 'sysctl' command as a subprocess is not very elegant but I couldn't find a nice built-in Python function to determine if this is an Arm based mac. I tried the following. As you can see, the results on an Intel mac were the same as for the Intel version of Python on an M1 mac:

Intel Mac M1 mac, Python 3.9.13 Intel-only installer M1 mac Python 3.9.13 universal2 installer
platform.architecture() ('64bit', '') ('64bit', '') ('64bit', '')
platform.machine() 'x86_64' 'x86_64' 'arm64'
platform.platform() 'macOS-10.16.x86_64-i386-64bit' 'macOS-10.16.x86_64-i386-64bit' 'macOS-12.6.1-arm64-arm-64bit'
platform.processor() 'i386' 'i386' 'arm'
platform.system() 'Darwin' 'Darwin' 'Darwin'

The 'sysctl' command returns what we need:

Intel Mac M1 mac, Python 3.9.13 Intel-only installer M1 mac Python 3.9.13 universal2 installer
subprocess.run(['sysctl', '-n', 'machdep.cpu.brand_string'] Intel(R) Core(TM) i7-8750H CPU @ 2.20GHz Apple M1 Apple M1

If anyone can think of a cleaner way to determine if this is Intel based Python running on an Arm mac please shout. I got my solution after looking at the following:

https://www.youtube.com/watch?v=_uzZ5vD4uNw https://stackoverflow.com/questions/72888632/how-to-check-if-python-is-running-on-an-m1-mac-even-under-rosetta https://stackoverflow.com/questions/66955938/python-platform-on-m1-confusion https://stackoverflow.com/questions/72761210/c-mac-m1-intel-how-to-get-cpu-architecture https://stackoverflow.com/questions/65259300/detect-apple-silicon-from-command-line

The main issue with the current clr-loader v0.2.4, using 'universal2' Python installation is that if the user has both x64 .NET and Arm64 .NET installed it will fail with exception RuntimeError: Could not find a suitable hostfxr library in /usr/local/share/dotnet/x64

This is because the code gets the wrong path '/usr/local/share/dotnet/x64':

    elif sys.platform == "darwin":
        if sys.maxsize > 2**32:  # is_64bits
            dotnet_root = Path("/usr/local/share/dotnet/x64")
        else:
            dotnet_root = Path("/usr/local/share/dotnet")

If the user only has Arm64 .NET installed it will work because, even though it gets the wrong path '/usr/local/share/dotnet/x64' it then goes on to check if that directory exists, and because it doesn't exist, it uses find_dotnet_cli() to find the path, which uses shutil.which("dotnet"), which works:

    if dotnet_root is not None and dotnet_root.is_dir():
        return dotnet_root

    # Try to discover dotnet from PATH otherwise
    dotnet_cli = find_dotnet_cli()
LeeDongGeon1996 commented 1 year ago

Hi, @DareDevilDenis . Sorry for late. I have thought about applying your amazing workaround and no doubt that it's the most precise way to find dotnet root. But I think it is too much, so how about this way (c29d3af)? Since we just need to search only 2 paths(/dotnet, /dotnet/x64), let's just iterate them.

DareDevilDenis commented 1 year ago

Hi @LeeDongGeon1996 I like that your solution is simple, however on an Arm mac, if the user has both Arm .Net and x64 .Net installed, and uses an ‘Intel only’ Python, your solution will look for .Net in /usr/local/share/dotnet. It will find .Net here and try to load it. This will fail with an exception because an ‘Intel only’ Python cannot load an Arm based .Net.

I’m not sure how important this limitation is though. Python 3.9 was the last version that had an ‘Intel only’ installer. From Python 3.10 it’s a ‘universal2’ installer which will work fine with your solution.

LeeDongGeon1996 commented 1 year ago

@DareDevilDenis Thank you for the answer. I think the situation you are worrying about can be covered by this logic. For Arm Mac users, it's normal to use Not-Intel-only Python, and that's why we search /usr/local/share/dotnet first. And for the users who use Arm Mac and Intel-only Python, they must have only x64 .Net installed because, for them, Arm .Net is useless. Then the problem is resolved.

If Intel-only Python users persist to keep both .Net versions, they can explicitly select dotnet root by passing dotnet_root param to load() method or setting env variable like PYTHONNET_CORECLR_DOTNET_ROOT (ref). (<- Is it possible? @filmor )

DareDevilDenis commented 1 year ago

Hi @LeeDongGeon1996 Your reasoning makes sense to me. I think your code is ok :)

LeeDongGeon1996 commented 1 year ago

Hi, @filmor . If you're okay, we'd like to merge this PR. Before that, Could you check why test failed?

filmor commented 1 year ago

I'm not quite sure I follow the latest simplification. Depending on the .NET 6 structure is fine by me, but I don't want to have people knocking here (and in particular on Python.NET) over and over because their x64-Python fails to load .NET on their Apple Silicon install.

In summary:

    elif sys.platform == "darwin":
        if "ARM64" in os.uname().version:
            # Apple Silicon
            if platform.machine() == 'x86_64':
                # Running in Rosetta 2 mode
                dotnet_root = Path("/usr/local/share/dotnet/x64")
            else:
                dotnet_root = Path("/usr/local/share/dotnet")
        else:
            # Intel Silicon
            if sys.maxsize > 2**32:  # is_64bits
                dotnet_root = Path("/usr/local/share/dotnet/x64")
            else:
                dotnet_root = Path("/usr/local/share/dotnet")

What do you say, does this work?

DareDevilDenis commented 1 year ago

@filmor, I can try your code in a few days (I don't have access to a mac at the moment as I'm on holiday). I did notice that https://docs.python.org/3/library/os.html#os.uname says "Availability: Unix" so I'm not sure if it works on macOS.

DareDevilDenis commented 1 year ago

@filmor, in the code in your last comment, and in clr-loader 0.2.4, I don't think the following logic is correct:

if sys.maxsize > 2**32:  # is_64bits
    dotnet_root = Path("/usr/local/share/dotnet/x64")
else:
    dotnet_root = Path("/usr/local/share/dotnet")

For macOS .NET 6.0 onwards is only available in x64 and Arm64 versions. Previous to .NET 6 it's only available in x64. As far as I can see there is no 32 bit 'x86' version of .NET for macOS.

As we've discussed above, on macOS, .NET gets installed to:

.NET 6 onwards:

Before .NET 6:

On a 64 bit Python installation, the 'if sys.maxsize > 2**32:' check sets the path to usr/local/share/dotnet/x64. This is wrong for all cases except "Arm CPU, x64 .NET". The only reason why this wrong logic works in clr-loader 0.2.4 is because after getting the wrong path, it then goes on to check if that directory exists, and because it doesn't exist, it uses find_dotnet_cli() to find the path, which uses shutil.which("dotnet"), which works:

if dotnet_root is not None and dotnet_root.is_dir():
    return dotnet_root

# Try to discover dotnet from PATH otherwise
dotnet_cli = find_dotnet_cli()

So I don't think we need the 'if sys.maxsize > 2**32:' check.

filmor commented 1 year ago

Okay, then I'd suggest:

    elif sys.platform == "darwin":
        if "ARM64" in os.uname().version and platform.machine() == 'x86_64':
            # Apple Silicon in Rosetta 2 mode
            dotnet_root = Path("/usr/local/share/dotnet/x64")
        else:
            dotnet_root = Path("/usr/local/share/dotnet")

This should work in all cases you listed, right?

DareDevilDenis commented 1 year ago

Yes, I think that will be perfect :)

I tried os.uname().version on my Intel mac and it returned what we expected: 'Darwin Kernel Version 17.7.0: Mon Aug 31 22:11:23 PDT 2020; root:xnu-4570.71.82.6~1/RELEASE_X86_64' Hopefully I'll get access to an M1 mac tomorrow.

DareDevilDenis commented 1 year ago

@filmor, I tried on an M1 mac and got the expected output from os.uname().version (got the same result for Intel based Python and 'universal2' Python):

'Darwin Kernel Version 22.2.0: Fri Nov 11 02:04:44 PST 2022; root:xnu-8792.61.2~4/RELEASE_ARM64_T8103'

So your logic is good 😀

One small suggestion: I think that we should switch the clauses in the ifstatement, from:

if "ARM64" in os.uname().version and platform.machine() == 'x86_64':

To:

if platform.machine() == 'x86_64' and "ARM64" in os.uname().version:

The reason is that since Apple have moved to using their own silicon that will soon become the processor used on most mac machines. Our if check will get a small performance increase (very small I know, but hey, every little helps!) on an ARM based mac with a 'universal2' Python installation because lazy evaluation will stop checking when platform.machine() == 'x86_64' returns False.

filmor commented 1 year ago

Superceded by #45, thank you both for your efforts :)