Update blob.py #390

BandungBB · 2023-08-10T15:47:59Z

This fix works for any installation of ZODB on unspecified platforms like Android. It is proposed for the termux version of ZODB.

While a pip install zodb within the termux distro for Android completes without incident, it fails to run since any attempts to import ZODB within termux's python fails. This is because their python's os.link has been disabled for some cross platform reason related to Windows.

The only ZODB library code which uses os.link is in ZODB.blob.py where it is selected as the value for the link_or_copy name on Posix systems. On Windows systems, it is mapped to shutil.copy.

This pull request extends the mapping of link_or_copy to shutil.copy for Android, not by attempting to identify the platform as Android, but by attempting a try:except snippet.

Closes #257.

This fix works for any installation of ZODB on unspecified platforms like Android. It is proposed for the termux version of ZODB. While a pip install zodb within the termux distro for Android completes without incident, it fails to run since any attempts to import ZODB within termux's python fails. This is because their python's os.link has been disabled for some cross platform reason related to Windows. The only ZODB library code which uses os.link is in ZODB.blob.py where it is selected as the value for the link_or_copy name on Posix systems. On Windows systems, it is mapped to shutil.copy. This pull request extends the mapping of link_or_copy to shutil.copy for Android, not by attempting to identify the platform as Android, but by attempting a try:except snippet.

tseaver · 2023-08-12T03:00:12Z

src/ZODB/blob.py

+    try:
+        link_or_copy = shutil.copy
+    except AttributeError:    #pragma: no cover
+        link_or_copy = shutil.copy


First, this cannot be the correct spot for the try: .. except: ... bit: it is inside the if sys.platform == "win32": block. The correct location should be below (line 950).

Second, the two parts of the try: ... except: ... test must not have the same content. Line 950 below should be changed to:

try: link_or_copy = os.link except AttributeError: #pragma: no cover link_or_copy = shutil.copy

Ooops. You are correct. The changes were meant for line 950

This change fixes the location for the try: except clause. I meant to place it after the if win32 check.

tseaver · 2023-08-13T14:07:36Z

src/ZODB/blob.py

@@ -942,13 +942,14 @@ def remove_committed_dir(path):
                filename = os.path.join(dirpath, filename)
                remove_committed(filename)
        shutil.rmtree(path)
-
-    link_or_copy = shutil.copy
+        link_or_copy = shutil.copy


This assignment must be at the same indentation level as the function definitions: it is not part of the function.

Suggested change

link_or_copy = shutil.copy

link_or_copy = shutil.copy

tseaver · 2023-08-13T14:09:51Z

src/ZODB/blob.py

+    except AttributeError:    #pragma: no cover
+        link_or_copy = shutil.copy


Please add a comment indicating why we fall back to shutil.copy here, e.g.:

Suggested change

except AttributeError: #pragma: no cover

link_or_copy = shutil.copy

except AttributeError: #pragma: no cover

# FBO termux on Android.

# See https://github.com/zopefoundation/ZODB/issues/257

link_or_copy = shutil.copy

Ok. Just added those comments. I tried to see how the proposed change to blob.py within ZODB works with other python implementations like for iSH for IOS which has Alpine linux as it's underpinnings. Unfortunately, their python is so broken with many dependencies not available that libraries such as ZODB won't even install.

Added notes that point to issue 257. This change is required in order for ZODB to be imported on termux's python implementation which has disables os.link due to compatibility issues with their win32 implementation.

dataflake · 2023-08-17T05:38:25Z

Please take a look at the GitHub Actions test run. There's some linting issues and this PR breaks all Windows tests.

fixed the two lint errors wrt incorrect usage of # - added space: #pragma to: # pragma - removed extra space: after # see https:// to: # see https://

- fixed the lint errors wrt the use of '#' comments - fixed link_or_copy not being set for any python on Windows, possibly due to the check for sys.platform == 'win32' failing because maybe the sys.platform == 'win64' as an example, rather than 'win32'.

missed the 2 blank lines, lint error

Fixed indent error within if sys.platform == 'win32' for the link_or_copy variable. This should fix the failing python on windows Indented the try: except statements again for the 'else"

dataflake · 2023-08-17T09:19:31Z

There's still a linter failure:

lint: commands[1]> flake8 src setup.py
src/ZODB/blob.py:945:1: W293 blank line contains whitespace
src/ZODB/blob.py:954:64: W291 trailing whitespace

whitespace removed from line 954

src/ZODB/blob.py

tseaver · 2023-08-18T17:24:38Z

src/ZODB/blob.py

        shutil.rmtree(path)
-
    link_or_copy = shutil.copy


One last niggle: the empty line separates a function definition from the assignment, so please restore it.

Line added.

blank space removed from empty line.

BandungBB · 2023-08-27T03:35:54Z

Why would a 5 line fix that is nothing more than a try/except wrapper around the setting of a variable, trigger a Sphinx extension loading error like this one?

Could not import extension sphinxcontrib.zopeext.autointerface (exception: cannot import name 'ObjectMembers' from 'sphinx.ext.autodoc' (/home/runner/work/ZODB/ZODB/.tox/docs/lib/python3.9/site-packages/sphinx/ext/autodoc/init.py))

What if I were to destroy my fork of zodb , create another fork, create a pull request on the new fork which includes the recommended change as one clean commit? Could it be that the fork is the problem?

d-maurer · 2023-08-27T05:54:02Z

Bandung wrote at 2023-8-26 20:36 -0700:

Why would a 5 line fix that is nothing more than a try/except wrapper around the setting of a variable, trigger a Sphinx extension loading error like this one? Could not import extension sphinxcontrib.zopeext.autointerface (exception: cannot import name 'ObjectMembers' from 'sphinx.ext.autodoc' (/home/runner/work/ZODB/ZODB/.tox/docs/lib/python3.9/site-packages/sphinx/ext/autodoc/__init__.py))

This looks like a version mismatch problem. A missing "version pin" is the main cause for such problems. The missing version pin can result in the installation of different versions in different environments. Without a version pin, incompatibilities between a newer and an older version occasionally let newer test runs fail that have succeeded earlier. The test failure you observe has nothing to do with your change. It affects only the documentation generation. The cause is likely an incompatibility of a newer `sphinx` version (the documentation generation tool). When I hit such a case and do not see an obvious fix, I (mostly) ignore the error, write a PR comment for people more familiar with the respective failure area and then continue in the normal PR process.

tseaver requested changes Aug 12, 2023

View reviewed changes

Update blob.py

05221e2

This change fixes the location for the try: except clause. I meant to place it after the if win32 check.

tseaver requested changes Aug 13, 2023

View reviewed changes

Update blob.py

91fab40

Added notes that point to issue 257. This change is required in order for ZODB to be imported on termux's python implementation which has disables os.link due to compatibility issues with their win32 implementation.

BandungBB added 4 commits August 17, 2023 01:44

Update blob.py

b66e07e

fixed the two lint errors wrt incorrect usage of # - added space: #pragma to: # pragma - removed extra space: after # see https:// to: # see https://

Update blob.py

fe5e319

- fixed the lint errors wrt the use of '#' comments - fixed link_or_copy not being set for any python on Windows, possibly due to the check for sys.platform == 'win32' failing because maybe the sys.platform == 'win64' as an example, rather than 'win32'.

Update blob.py

0e2176e

missed the 2 blank lines, lint error

Update blob.py

40975ad

Fixed indent error within if sys.platform == 'win32' for the link_or_copy variable. This should fix the failing python on windows Indented the try: except statements again for the 'else"

Update blob.py

0cba2bb

whitespace removed from line 954

icemac reviewed Aug 18, 2023

View reviewed changes

src/ZODB/blob.py Outdated Show resolved Hide resolved

Update src/ZODB/blob.py

8e4591a

tseaver requested changes Aug 18, 2023

View reviewed changes

BandungBB added 2 commits August 18, 2023 18:27

Update blob.py

a17af3c

Line added.

Update blob.py

8ae4188

blank space removed from empty line.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update blob.py #390

Update blob.py #390

BandungBB commented Aug 10, 2023 •

edited by tseaver

Loading

tseaver Aug 12, 2023

BandungBB Aug 12, 2023

tseaver Aug 13, 2023

tseaver Aug 13, 2023

BandungBB Aug 16, 2023

dataflake commented Aug 17, 2023

dataflake commented Aug 17, 2023

tseaver Aug 18, 2023

BandungBB commented Aug 27, 2023

d-maurer commented Aug 27, 2023 via email

		except AttributeError: #pragma: no cover
		link_or_copy = shutil.copy

Update blob.py #390

Are you sure you want to change the base?

Update blob.py #390

Conversation

BandungBB commented Aug 10, 2023 • edited by tseaver Loading

tseaver Aug 12, 2023

Choose a reason for hiding this comment

BandungBB Aug 12, 2023

Choose a reason for hiding this comment

tseaver Aug 13, 2023

Choose a reason for hiding this comment

tseaver Aug 13, 2023

Choose a reason for hiding this comment

BandungBB Aug 16, 2023

Choose a reason for hiding this comment

dataflake commented Aug 17, 2023

dataflake commented Aug 17, 2023

tseaver Aug 18, 2023

Choose a reason for hiding this comment

BandungBB commented Aug 27, 2023

d-maurer commented Aug 27, 2023 via email

BandungBB commented Aug 10, 2023 •

edited by tseaver

Loading