Skip to content

Support ARM architectures again #887

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

Merged
merged 1 commit into from
Jun 24, 2019
Merged

Conversation

filmor
Copy link
Member

@filmor filmor commented Jun 20, 2019

What does this implement/fix? Explain your changes.

Adds ARM versions to the native page hack used to survive app-domain reloading.

Does this close any currently open issues?

#873

@filmor
Copy link
Member Author

filmor commented Jun 20, 2019

@benoithudson, could you have a look?
@bimajatiwijaya, could you check whether this branch works for you?

Copy link
Contributor

@benoithudson benoithudson left a comment

Choose a reason for hiding this comment

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

Looks great! Glad to see the framework is indeed extensible.

Copy link
Contributor

@benoithudson benoithudson left a comment

Choose a reason for hiding this comment

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

This is the right style; I don't have an ARM handy to test it on.

I'm assuming you're on some variant of ARM/linux?

@filmor
Copy link
Member Author

filmor commented Jun 20, 2019

The only thing I have is an old Raspberry Pi and a few phones, I just used godbolt to get the binaries. That's why I'd like people that had problems to test this branch ;)

@codecov-io
Copy link

Codecov Report

Merging #887 into master will increase coverage by 0.08%.
The diff coverage is 93.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #887      +/-   ##
==========================================
+ Coverage   76.86%   76.94%   +0.08%     
==========================================
  Files          64       64              
  Lines        5909     5939      +30     
  Branches      974      974              
==========================================
+ Hits         4542     4570      +28     
- Misses       1040     1042       +2     
  Partials      327      327
Flag Coverage Δ
#setup_linux 65.3% <ø> (ø) ⬆️
#setup_windows 76.17% <93.33%> (+0.08%) ⬆️
Impacted Files Coverage Δ
src/runtime/runtime.cs 86.91% <100%> (+0.07%) ⬆️
src/runtime/typemanager.cs 83.64% <92.85%> (+0.87%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2bc514f...d834b65. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Jun 20, 2019

Codecov Report

Merging #887 into master will decrease coverage by 13.94%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #887       +/-   ##
===========================================
- Coverage   76.86%   62.92%   -13.95%     
===========================================
  Files          64        1       -63     
  Lines        5909      294     -5615     
  Branches      974        0      -974     
===========================================
- Hits         4542      185     -4357     
+ Misses       1040      109      -931     
+ Partials      327        0      -327
Flag Coverage Δ
#setup_linux ?
#setup_windows 62.92% <ø> (-13.17%) ⬇️
Impacted Files Coverage Δ
setup.py 62.92% <0%> (-24.15%) ⬇️
src/runtime/pyansistring.cs
src/runtime/constructorbinder.cs
src/runtime/methodbinder.cs
src/runtime/Util.cs
src/runtime/delegatemanager.cs
src/runtime/eventbinding.cs
src/runtime/pyiter.cs
src/runtime/pynumber.cs
src/runtime/interop27.cs
... and 52 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2bc514f...d834b65. Read the comment docs.

@amos402
Copy link
Member

amos402 commented Jun 21, 2019

Ummm, I think it seems that it would better if we create a dynamic library and use P/Invoke to get a function address rather than making some hard codes like these?

@filmor
Copy link
Member Author

filmor commented Jun 21, 2019

That would complicate the build process quite a lot though. I'd really like to get to a single arch independent assembly in the future that can just be compiled using dotnet cli without any additional params.

@amos402
Copy link
Member

amos402 commented Jun 21, 2019

But at first, you always need to build a sample binary and disassembly to get codes depend the system.
If you worry about the build process, why not just upload that prebuilt binary to repository, so that users would not need any compile.
What if we need to compatible systems like these:

How complicated would typemanager.cs be......
Well, for now, this is not a big problem because there are only two dummy functions.

@filmor
Copy link
Member Author

filmor commented Jun 21, 2019

The trick is that it's straight binary, so you only need one of these per cpu arch, not per os (no ELF vs PE distinction, no libc linking etc). Be my guest in providing a reasonable alternative.

@amos402
Copy link
Member

amos402 commented Jun 21, 2019

Oh, it is. I almost neglect it.

@benoithudson
Copy link
Contributor

That would complicate the build process quite a lot though. I'd really like to get to a single arch independent assembly in the future that can just be compiled using dotnet cli without any additional params.

Me too, which is why my patch that introduced this code was discovering the platform at runtime rather than with ifdefs.

If there's some rainy days I'll see about converting more of the code. Eliminating the MONO_LINUX and MONO_OSX defines would be nice; UCS as well.

@amos402
Copy link
Member

amos402 commented Jun 21, 2019

By the way, the implementation for Runtime.InitializePlatformData just have some limitations.
The platform.py would import subprocess, but not all release binaries have it, I encounter it before and use ifdef skiped, any good suggestion?

@benoithudson
Copy link
Contributor

Probably inline the two functions. IIRC one of the calls has a CPython component, the other I didn't track down.

We're getting pretty far off the original topic, I wonder if comments on this PR are the best way to gather our thoughts?

@filmor filmor merged commit fc7d8a4 into pythonnet:master Jun 24, 2019
@filmor filmor deleted the support-arm branch May 18, 2020 11:29
AlexCatarino pushed a commit to QuantConnect/pythonnet that referenced this pull request Jun 18, 2020
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.

4 participants