Skip to content
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

[#132] Base py3 migration. #676

Merged
merged 58 commits into from
Jul 4, 2023
Merged

[#132] Base py3 migration. #676

merged 58 commits into from
Jul 4, 2023

Conversation

adiroiban
Copy link
Member

@adiroiban adiroiban commented Jan 27, 2022

Scope

Fix #132

This is the migration of the compat code to py3.

It removes py2 support as we don't want to look back :)

Changes

NT and Unix services compat were removed as they are no longer used.
We now use external tools to manage our daemons.

NT command line unicode converting code was removed as it's no longer an issue with py3.

The package definition was "modernized" to declare everyting inside setup.cfg

the package was moved outside of the namespace package.
With a new package name, in theory we can have both old and new compat installed at the same time.
it was also moved into a dedicates 'src' path.

The change was easy as we have already prepared most of the compat code for py3.

How to try and test the changes

reviewers: @danuker

check that changes make sense.

We will know for sure if this work, once we integrated it with chevah-server

@adiroiban adiroiban force-pushed the 132-py3-migration branch from 660f0de to 6cda862 Compare May 23, 2023 18:13
@adiroiban adiroiban force-pushed the 132-py3-migration branch 3 times, most recently from 32ad757 to b8277fb Compare May 23, 2023 21:36
@adiroiban adiroiban force-pushed the 132-py3-migration branch from b8277fb to 0acc6f8 Compare May 23, 2023 21:39
@adiroiban
Copy link
Member Author

From Mișu


i guess the ones with glibc older than 2.26 should be removed when switching to a newer py3 version:

10:13 amazon 2018.03, centos 5/6/7, ubuntu 14.04/16.04
10:15 and maybe also bump the version of alpine to 3.18, no other changes should be required for it

@adiroiban adiroiban force-pushed the 132-py3-migration branch from b9ab562 to d4cbe39 Compare May 24, 2023 13:53
@adiroiban adiroiban force-pushed the 132-py3-migration branch from d4cbe39 to ddd6aa3 Compare May 24, 2023 14:04
@adiroiban adiroiban force-pushed the 132-py3-migration branch from b2b9065 to a56c5e6 Compare May 24, 2023 15:15
@adiroiban
Copy link
Member Author

python3 bare tests are green... I guess we can switch to 3.11 :)

@adiroiban adiroiban changed the title [#132] Initial py3 migration, [#132] Base py3 migration. May 24, 2023
@adiroiban
Copy link
Member Author

I am going to merge this... the future is py3.

We can create a separate branch is we need py2 backward compatibility releases

@adiroiban
Copy link
Member Author

Dan, please take a quick look at this.

The main changes are tested in chevah/server but we also need a good test coverage here.

note that this is the initial py3.8 preparation work.

The final green test run is with python 3.11 at #693

needs-review

Copy link
Contributor

@danuker danuker left a comment

Choose a reason for hiding this comment

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

Glad we can get rid of a lot of Unicode/Windows special cases.
I only had minor comments.

; Required for some unicode handling.
unidecode

wmi ~= 1.5.1; platform_system == "Windows"
Copy link
Contributor

Choose a reason for hiding this comment

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

; platform_system == "Windows"

Is this comment also code?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes... this is code...
https://setuptools.pypa.io/en/latest/userguide/dependency_management.html#platform-specific-dependencies

I guess that we should migrate to .toml files ... but in a separate PR.

@@ -763,6 +756,11 @@ def _dirEntryToFileAttributes(self, entry):
if path.startswith('\\\\?\\') and path[5] == ':':
path = path[4:]

hardlinks = stats.st_nlink
if not hardlinks and os.name == 'nt':
# I don't know why on Windows we doing scandir.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe speed?

Using scandir() increases the speed of os.walk() by 2-20 times (depending on the platform and file system) by avoiding unnecessary calls to os.stat() in most cases.

From the package that is now included in Py3.

Copy link
Member Author

Choose a reason for hiding this comment

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

The comment was bad... the comment was about scandir returning hardlinks=0

we use scandir for speed and most importantly not to block the process.

With os.listdir, if you have 100.000 files in a dir, when you call it it will return all of them in one go, possible blocking the process.

with scandir we have an iterator

# On Windows the folder will have a size.
# So we skip this check on Windows.
result = self.filesystem.getFileSize(segments)
self.assertEqual(0, result)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be any use to check the actual size of the file linked, in the case of Windows?

if not isinstance(tail, text_type):
tail = tail.decode('utf-8')
if not isinstance(tail, str):
tail = tail
Copy link
Contributor

Choose a reason for hiding this comment

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

This line does nothing

@adiroiban adiroiban merged commit 22902d9 into master Jul 4, 2023
@adiroiban adiroiban deleted the 132-py3-migration branch July 4, 2023 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate compat to Py3
4 participants