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

WIP: Python2 support #18

Closed
wants to merge 5 commits into from
Closed

Conversation

fridex
Copy link
Collaborator

@fridex fridex commented Feb 27, 2020

This introduces a breaking change

  • Yes
  • No

Fixes: #19

@sefkhet-abwy sefkhet-abwy bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 27, 2020
@ghost
Copy link

ghost commented Feb 27, 2020

Build failed.

Copy link
Member

@harshad16 harshad16 left a comment

Choose a reason for hiding this comment

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

"""Find and load Pipfile.lock"""

can you add a period (.) at the end of the line. :)

@fridex
Copy link
Collaborator Author

fridex commented Feb 28, 2020

"""Find and load Pipfile.lock"""

can you add a period (.) at the end of the line. :)

#21

@harshad16
Copy link
Member

recheck

@ghost
Copy link

ghost commented Feb 28, 2020

Build failed.

@frenzymadness
Copy link
Collaborator

There are more issues than I thought. For example:

  • tests have hardcoded "python3", "pip3"
  • redirect_stdout has to be reimplemented in a similar way as cwd (does not exist on Python 2)
  • mypy is not supported on Python 2

I've ported a part of the codebase (see the latest commits) and now it seems that it partially works. Some tests also pass (21 failed, 18 passed, 1 error) and it seems to me that the remaining tests fail on differences between actual vs. expected output, for example:

>               assert f.read() == expected
E               assert "--index-url ...rsion < '4'\n" == "--index-url h...rsion < '4'\n"
E                   --index-url https://pypi.org/simple
E                   --extra-index-url https://tensorflow.pypi.thoth-station.ninja/
E                   --trusted-host tensorflow.pypi.thoth-station.ninja
E                   #
E                   # Default dependencies
E                   #
E                 + certifi==2019.11.28
E                 + chardet==3.0.4
E                   idna==2.8
E                   requests==2.22.0
E                 - certifi==2019.11.28
E                 - chardet==3.0.4

This has to be investigated more.

It would be also very nice to add tox configuration and move the pytest settings from setup.py to tox.ini/pyproject.toml - in a separated PR before we finish this one as it helps with testing and setting environments in a different way for Py2 and Py3.

But, first of all, as it seems that it won't be that easy, do you still want to support Python 2?

@ghost
Copy link

ghost commented Mar 2, 2020

Build failed.

@fridex
Copy link
Collaborator Author

fridex commented Mar 2, 2020

@frenzymadness thanks for the work on this! 👍

But, first of all, as it seems that it won't be that easy, do you still want to support Python 2?

We don't have Python2 interest, it's EOL for us. Would Python2 efforts on your (Python s2i) front make anything easier?

@frenzymadness
Copy link
Collaborator

I completely understand you. I spent years making software compatible with Python 3 and I don't want to work other way around. I've summarized our possibilities in sclorg/s2i-python-container#368

@fridex
Copy link
Collaborator Author

fridex commented Mar 12, 2020

@frenzymadness I will close this based on our meeting today. Let's keep it in the history of Internet if we would change our minds...

Thanks for your time on this.

@fridex fridex closed this Mar 12, 2020
@fridex fridex deleted the python2-support branch March 12, 2020 13:15
@fridex fridex mentioned this pull request Mar 12, 2020
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python2 support
3 participants