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

Solution to issue #133 when building for 32bit and 64bit #135

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

HanslettTheDev
Copy link
Collaborator

@HanslettTheDev HanslettTheDev commented Aug 23, 2023

Summary

This PR solves the issue #133 when installing pillow on 64bit systems
After much research, it seems it's not possible to specify the Python version type directly

What we specify in the GH yaml file is not the OS type but rather the python type. So we are running both 32bit and 64bit python versions on a 64bit processor(or GH runner).

runs-on: windows-latest
    strategy:
      fail-fast: false
      matrix:
        python-version: ["3.10"]
        targetplatform: [x86, x64]

    steps:
    - uses: actions/checkout@v2
      with:
        fetch-depth: 0
    - name: Set up Python ${{ matrix.python-version }}
      uses: actions/setup-python@v2
      with:
        python-version: ${{ matrix.python-version }}
        architecture : ${{ matrix.targetplatform }}

Based on their GH action documentation on runners, all runners run on 64bit systems.

A Walkaround is to set the requirements.txt always to use the latest version of Pillow and then change the requirements.txt file before installing the requirements on windows 32bit Python versions so it compiles successfully for 32bit builds while using the latest version for 64bit

- name: Change requirements.txt on 32bit python
      if: matrix.targetplatform == 'x86'
      run: |
        python -c "with open('requirements.txt', 'r') as file: content = file.read(); content = content.replace('Pillow>=2.0.0', 'Pillow<=9.5.0'); f = open('requirements.txt', 'w'); f.write(content)"

MANUAL TEST EVIDENCE

Pillow Installed on 32bit python don't exceed v9.5.0
image

while Pillow on 64bit use the latest versions
image

all this while ensuring the normal python 3+ tests are running

Checklist

  • Classes, Variables, function and methods logic ok

Signed-off-by: HanslettTheDev <HanslettTheDev@gmail.com>
…ons on the fly

Signed-off-by: HanslettTheDev <HanslettTheDev@gmail.com>
Copy link
Member

@reingart reingart left a comment

Choose a reason for hiding this comment

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

Interesting investigation, thanks!
As you mention, PEP-508 seems to lack a variable with the python interpreter complete description...

sys.version could help as it has the compiler identification 3.9.13 (tags/v3.9.13:6de2ca5, May 17 2022, 16:36:42) [MSC v.1929 64 bit (AMD64)], but I don't know it could be used or if there is another workaround.

I would prefer to detect windows and avoid special parsing in requirements.
For example you could add and sys_platform != 'win32' for the 64 bit version (or linux, not tested there...)

@HanslettTheDev
Copy link
Collaborator Author

Now here is another problem, still based on the PEP0508 Standards, when we use sys_platform, it internally calls sys.platform. Now even on 64bit computers, the expected result is also "win32". The reason is due to the fact 64bit processors can still run 32bit instructions, and for the sake of backward compatibility on 32bit python, the expected result of sys.platform regardless on whether it is 64bit or 32bit will always be "win32".

I use a 64bit computer, and here is the output below
image

Shows "win32" though I am running on a 64bit processor.
Here is a StackOverflow question which gives insight on the behaviour

SInce you want to avoid anything with touching the requirements,txt, we can use sys.maxsize to determine if the python version runs on 64bit or 32bit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants