GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Add loongarch64 support by xiangzhai · Pull Request #4914 · RustPython/RustPython

Conversation

xiangzhai commented Apr 21, 2023

Copy link

Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Add loongarch64 support

77a9071

youknowone requested changes

Apr 21, 2023

View reviewed changes

youknowone left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Related CPython issues
ref

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/test/test_sysconfig.py Outdated

		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):

youknowone Apr 21, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

xiangzhai Apr 23, 2023

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          It doesn't need to change test_triplet_in_ext_suffix

fe8a72c

youknowone approved these changes

Apr 23, 2023

View reviewed changes

youknowone left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thank you!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

xiangzhai reacted with heart emoji

All reactions

❤️ 1 reaction

xiangzhai added 2 commits

April 23, 2023 10:54


          Fix cargo fmt check

cb675f7


          Fix cargo clippy -Dwarnings unused import warning issue

f7bb7d7

xiangzhai requested a review from youknowone

April 23, 2023 03:19

xiangzhai commented Apr 23, 2023

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

youknowone reviewed

Apr 23, 2023

View reviewed changes

Comment thread

stdlib/src/mmap.rs Outdated

		MADV_MERGEABLE, MADV_NOHUGEPAGE, MADV_REMOVE, MADV_UNMERGEABLE,
		};

		#[allow(unused_imports)]

youknowone Apr 23, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

xiangzhai Apr 23, 2023

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Fixed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Place allow unused imports under cfg

ee3e23a

youknowone reviewed

Apr 23, 2023

View reviewed changes

Comment thread

stdlib/src/mmap.rs Outdated

		)
		)
		))]
		#[allow(unused_imports)]

youknowone Apr 23, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

xiangzhai Apr 23, 2023

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I got :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Replace allow unused_imports with pyattr

c9f1709

xiangzhai requested a review from youknowone

April 23, 2023 03:48

youknowone merged commit 2c90b12 into RustPython:main

Apr 23, 2023

youknowone commented Apr 23, 2023

Copy link

Copy Markdown

Member

Thank you for contributing!

xiangzhai reacted with heart emoji

All reactions

❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Reviewers

youknowone

Awaiting requested review from youknowone

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

xiangzhai commented Apr 21, 2023

Uh oh!

youknowone left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowone Apr 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiangzhai Apr 23, 2023

Choose a reason for hiding this comment

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

xiangzhai commented Apr 23, 2023

Uh oh!

youknowone Apr 23, 2023

Choose a reason for hiding this comment

Uh oh!

xiangzhai Apr 23, 2023

Choose a reason for hiding this comment

Uh oh!

youknowone Apr 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiangzhai Apr 23, 2023

Choose a reason for hiding this comment

Uh oh!

youknowone commented Apr 23, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

youknowone left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

youknowone Apr 21, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

youknowone Apr 23, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.