Skip to content

Add loongarch64 support #4914

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Apr 23, 2023
Merged

Add loongarch64 support #4914

merged 6 commits into from
Apr 23, 2023

Conversation

xiangzhai
Copy link
Contributor

Hi,

cargo build && test, all tests pass:

     Running unittests src/lib.rs (target/debug/deps/rustpython-2c061bc00c2f70a2)

running 1 test
Hello
Hello
test tests::test_run_script ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.38s

     Running unittests src/main.rs (target/debug/deps/rustpython-f9242e878d797bdd)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

   Doc-tests rustpython

running 1 test
test src/lib.rs - (line 8) - compile ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.15s

Please review my patch.

Thanks,
Leslie Zhai

youknowone
youknowone requested changes Apr 21, 2023
View reviewed changes
Copy link
Member

@youknowone youknowone left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Related CPython issues
ref

Lib/test/test_sysconfig.py Outdated
@@ -499,7 +499,7 @@ def test_triplet_in_ext_suffix(self):
import platform, re
machine = platform.machine()
suffix = sysconfig.get_config_var('EXT_SUFFIX')
if re.match('(aarch64|arm|mips|ppc|powerpc|s390|sparc)', machine):
if re.match('(aarch64|arm|loongarch64|mips|ppc|powerpc|s390|sparc)', machine):
Copy link
Member

@youknowone youknowone Apr 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We generally don't edit this file ourselves because it mostly came from CPython.
But I understand this is necessary to correctly support the architecture.
Not to lose the changes when we update this file to new version,
Adding a comment is useful.

Suggested change
if re.match('(aarch64|arm|loongarch64|mips|ppc|powerpc|s390|sparc)', machine):
# XXX: RUSTPYTHON; loongarch64 is locally added in github #4914
if re.match('(aarch64|arm|loongarch64|mips|ppc|powerpc|s390|sparc)', machine):

And also, I found the related CPython patch doesn't include this change. Is it necessary in RustPython?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your careful review!

It is not necessary in RustPython.

Fixed.

youknowone
youknowone approved these changes Apr 23, 2023
View reviewed changes
Copy link
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

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

Thank you!

@xiangzhai xiangzhai requested a review from youknowone April 23, 2023 03:19
@xiangzhai
Copy link
Contributor Author

Hi @youknowone

I fixed cargo fmt and clippy issues and ran the test for Ubuntu 22.04 x86_64:

cargo fmt --all -- --check
cargo clippy --no-default-features --features stdlib,zlib,importlib,encodings,ssl,jit -p rustpython-common -p rustpython-compiler-core -p rustpython-compiler -p rustpython-codegen -p rustpython-parser -p rustpython-vm -p rustpython-stdlib -p rustpython-jit -p rustpython-derive -p rustpython -- -Dwarnings

Please review it again!

Thanks,
Leslie Zhai

youknowone
youknowone reviewed Apr 23, 2023
View reviewed changes
stdlib/src/mmap.rs Outdated
};

#[allow(unused_imports)]
Copy link
Member

Choose a reason for hiding this comment

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

This is supposed to be used by #[pyattr]

Suggested change
#[allow(unused_imports)]
#[pyattr]

Because we usually use cfg - pyattr order, placing it under cfg will be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

youknowone
youknowone reviewed Apr 23, 2023
View reviewed changes
stdlib/src/mmap.rs Outdated
)
)
))]
#[allow(unused_imports)]
Copy link
Member

@youknowone youknowone Apr 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
#[allow(unused_imports)]
#[pyattr]

The value has to be used by pyattr to be exposed to python side

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got :)

@xiangzhai xiangzhai requested a review from youknowone April 23, 2023 03:48
@youknowone youknowone merged commit 2c90b12 into RustPython:main Apr 23, 2023
@youknowone
Copy link
Member

Thank you for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youknowone youknowone Awaiting requested review from youknowone

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

Successfully merging this pull request may close these issues.
2 participants
@xiangzhai @youknowone