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

feat: improved support of python3 #17

Merged
merged 4 commits into from
May 10, 2023

Conversation

knst
Copy link

@knst knst commented Apr 2, 2023

With modern python the setup process by an old instruction will trigger a warning:
setup.py:1: DeprecationWarning: The distutils package is deprecated and slated for removal in Python 3.12. Use setuptools or check PEP 632 for potential alternatives

To avoid this (and other related warnings) were updated:

  • package distutils.core replaced by modern setuptools
  • updated README.txt to use PIP instead call setup.py directly
  • used python3 in README.txt instead python

That gives 2 extra features beside rid of warnings:

  • you can easily install dash_hash package without root permission on host
  • you can uninstall package now by pip3/python-pip easily

@knst knst force-pushed the improve-python3-support branch 3 times, most recently from 7697ed5 to f90ed5c Compare April 2, 2023 09:19
With modern python running old instruction will trigger a warning:
    ```
    setup.py:1: DeprecationWarning: The distutils package is deprecated and slated for removal in Python 3.12. Use setuptools or check PEP 632 for potential alternatives
    ```

To avoid this (and other related warnings) were updated:
 - package `distutils.core` replaced by modern `setuptools`
 - updated README.txt to use PIP instead call setup.py directly
 - used python3 in README.txt instead python

That gives 2 extra features beside rid of warnings:
 - you can easily install dash_hash package without root permission on host
 - you can uninstall package now by pip3/python-pip easily
@knst knst marked this pull request as draft April 2, 2023 14:42
@knst knst marked this pull request as ready for review April 2, 2023 15:25
UdjinM6
UdjinM6 previously approved these changes Apr 3, 2023
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

ACK

PastaPastaPasta
PastaPastaPasta previously approved these changes Apr 4, 2023
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

utACK for squash merge; strong concept ack :) @thephez some docs will need updating

@thephez
Copy link

thephez commented Apr 4, 2023

@PastaPastaPasta This is the only reference I see in the dash repository: https://github.com/dashpay/dash/tree/master/test#dependencies-and-prerequisites. If you're aware of others, please share.

@knst
Copy link
Author

knst commented Apr 4, 2023

          @PastaPastaPasta This is the only reference I see in the dash repository: https://github.com/dashpay/dash/tree/master/test#dependencies-and-prerequisites. If you're aware of others, please share.

Originally posted by @thephez in #17 (comment)

I'll prepare PR for that repo too

@thephez
Copy link

thephez commented Apr 4, 2023

I'll prepare PR for that repo too

@knst You can build off dashpay/dash#5291 if more changes are needed

@thephez
Copy link

thephez commented Apr 4, 2023

Hmm, this is going to break too? https://github.com/dashpay/dash/blob/develop/contrib/containers/ci/Dockerfile#L47

@UdjinM6
Copy link

UdjinM6 commented Apr 4, 2023

since this is a breaking change it might make sense to bump version too (1.3.3 or 1.4)

@knst knst dismissed stale reviews from PastaPastaPasta and UdjinM6 via 542f1e8 April 5, 2023 08:48
@knst
Copy link
Author

knst commented Apr 5, 2023

As I just tested, upgrade from version 1.3.2 also works:

$ python3 setup.py install --user # option --user let to install in local directory by old script
$ git checkout 542f1e8
$ pip3 install . 
Defaulting to user installation because normal site-packages is not writeable
Processing /home/knst/projects/dash_hash
  Preparing metadata (setup.py) ... done
Building wheels for collected packages: dash-hash
  Building wheel for dash-hash (setup.py) ... done
  Created wheel for dash-hash: filename=dash_hash-1.4.0-cp310-cp310-linux_x86_64.whl size=573319 sha256=fb62b31b36b1bc71ef5e3f5e80ce53914af0b37b3e2365f5f4c1b8f97a626a88
  Stored in directory: /tmp/pip-ephem-wheel-cache-4q8v7fy2/wheels/10/42/16/3ed762cc124a59a29e5d839114a062f290c78c91142e1b6ba5
Successfully built dash-hash
Installing collected packages: dash-hash
  Attempting uninstall: dash-hash
    Found existing installation: dash-hash 1.3.2
    Uninstalling dash-hash-1.3.2:
      Successfully uninstalled dash-hash-1.3.2
Successfully installed dash-hash-1.4.0

@knst knst requested a review from UdjinM6 April 5, 2023 09:05
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

👍

utACK

@PastaPastaPasta PastaPastaPasta merged commit 7af3873 into dashpay:master May 10, 2023
8 checks passed
PastaPastaPasta pushed a commit to dashpay/dash that referenced this pull request May 12, 2023
## Issue being fixed or feature implemented
Install of dash_hash will change once
dashpay/dash_hash#17 is merged

## What was done?
- Changed install instructions to match new install in dash_hash README
- Updated Dockerfile to install correctly

## How Has This Been Tested?
N/A

## Breaking Changes
None

## Checklist:
<!--- Go over all the following points, and put an `x` in all the boxes
that apply. -->
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [x] I have made corresponding changes to the documentation
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request May 22, 2023
## Issue being fixed or feature implemented
Install of dash_hash will change once
dashpay/dash_hash#17 is merged

## What was done?
- Changed install instructions to match new install in dash_hash README
- Updated Dockerfile to install correctly

## How Has This Been Tested?
N/A

## Breaking Changes
None

## Checklist:
<!--- Go over all the following points, and put an `x` in all the boxes
that apply. -->
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [x] I have made corresponding changes to the documentation
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

4 participants