Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
filmor merged 1 commit intopythonnet:masterfromfilmor:support-arm
Jun 24, 2019
Merged

Conversation

filmor
Copy link
Member

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
MemberAuthor

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

Copy link
Contributor

@benoithudsonbenoithudson 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

@benoithudsonbenoithudson 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
MemberAuthor

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 intomaster willincrease coverage by0.08%.
The diff coverage is93.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
FlagCoverage Δ
#setup_linux65.3% <ø> (ø)⬆️
#setup_windows76.17% <93.33%> (+0.08%)⬆️
Impacted FilesCoverage Δ
src/runtime/runtime.cs86.91% <100%> (+0.07%)⬆️
src/runtime/typemanager.cs83.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 byCodecov. Last update2bc514f...d834b65. Read thecomment docs.

@codecov-io
Copy link

codecov-io commentedJun 20, 2019
edited
Loading

Codecov Report

Merging#887 intomaster willdecrease coverage by13.94%.
The diff coverage isn/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
FlagCoverage Δ
#setup_linux?
#setup_windows62.92% <ø> (-13.17%)⬇️
Impacted FilesCoverage Δ
setup.py62.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
... and52 more

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last update2bc514f...d834b65. Read thecomment docs.

@amos402
Copy link
Member

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
MemberAuthor

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 commentedJun 21, 2019
edited
Loading

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 wouldtypemanager.cs be......
Well, for now, this is not a big problem because there are only two dummy functions.

@filmor
Copy link
MemberAuthor

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

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 commentedJun 21, 2019
edited
Loading

By the way, the implementation forRuntime.InitializePlatformData just have some limitations.
Theplatform.py would importsubprocess, 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?

@filmorfilmor merged commitfc7d8a4 intopythonnet:masterJun 24, 2019
@filmorfilmor deleted the support-arm branchMay 18, 2020 11:29
AlexCatarino pushed a commit to QuantConnect/pythonnet that referenced this pull requestJun 18, 2020
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@benoithudsonbenoithudsonbenoithudson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@filmor@codecov-io@amos402@benoithudson

[8]ページ先頭

©2009-2025 Movatter.jp