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

Issue partial fix for #44: init pyproject.toml and poetry.lock #50

Merged
merged 6 commits into from
May 10, 2024

Conversation

scottrbrtsn
Copy link
Contributor

@scottrbrtsn scottrbrtsn commented Mar 1, 2024

for #44

Problem

There is a desire to become a pip package per Issue 44 linked above. It is recommended to also include a pyproject.toml so package managers like poetry can resolve dependencies. I recognize this is rather trivial to add for open-dis-python seeing as how (after I looked into it) there are virtually no external dependencies to create the package (only numpy. Not having a pyproject.toml could affect applications downstream desiring to easily add and manage opendis as a dependency.

However, there is concern with explicitly requiring python versions.

Solution

This PR initializes a pyproject.toml, a poetry lock (for the .venv) and sets the pyproject to use python 3.5 or higher.

Still unresolved

The issue in question could likely:

  1. remove the setup.py
  2. follow a pypi publish guide or with poetry publish
  3. then update the README to simply pip install opendis instead of git clone and pip install .

… created and runs as expected"

++ I realize deleting setup.py might upset some folks

This reverts commit 1a8cfa5.
pyproject.toml Outdated
name = "opendis"
version = "0.1.0"
description = ""
authors = ["Scott Robertson <[email protected]>"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a default, we can change this to the original authors of the package.

Copy link
Member

Choose a reason for hiding this comment

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

Yes let's change to the default authors in setup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@scottrbrtsn
Copy link
Contributor Author

Ah, I'm using my system python... tried on fresh machine and there are dependencies:

The current project's supported Python range (>=3.5,<4.0) is not compatible with some of the required packages Python requirement:

  • numpy requires Python >=3.9, so it will not be satisfied for Python >=3.5,<3.9

From RangeCoordinates

@ngjunsiang
Copy link
Contributor

Per https://github.com/numpy/numpy/releases/tag/v1.18.4, the latest version of numpy that supports Python 3.5 is v1.18.4, we'll have to version-lock that dependency to maintain Python 3.5 support

@leif81
Copy link
Member

leif81 commented Mar 1, 2024

Still unresolved

The issue in question could likely:

  1. remove the setup.py
  2. follow a pypi publish guide or with poetry publish
  3. then update the README to simply pip install opendis instead of git clone and pip install .

I'm good with these suggested changes as well. Let's move over values from the setup.py file into the new pyproject.toml file so they match as much as we can. Version, description, etc

@scottrbrtsn
Copy link
Contributor Author

Oldest python version available on my machine appears to be 3.6: https://wiki.archlinux.org/title/python

3.6 and 3.7 are tagged with "unmaintained". Seeing as how this is a DoD spec'ed protocol, shouldn't the community strongly consider upgrading?

https://www.cvedetails.com/vulnerability-list/vendor_id-16835/product_id-39445/Numpy-Numpy.html

@leif81
Copy link
Member

leif81 commented Mar 1, 2024

Another good point @scottrbrtsn
It is becoming an increasingly good case to set the baseline to Python 3.9. I support that direction now. If there any concerns please leave a comment here.

@leif81 leif81 linked an issue Mar 1, 2024 that may be closed by this pull request
@leif81 leif81 self-requested a review March 2, 2024 12:22
@leif81
Copy link
Member

leif81 commented Mar 2, 2024

Looking good.

Is there any benefit to keeping the setup.py if the use of poetry is merged here?

@scottrbrtsn
Copy link
Contributor Author

Looking good.

Is there any benefit to keeping the setup.py if the use of poetry is merged here?

I'm not 100% on details. Just that it would affect pip install behavior and folks might need adjust accordingly.

Similar example:
wearepal/ranzen#5

References:
python-poetry/poetry#321

@scottrbrtsn
Copy link
Contributor Author

Ah, this can be tested with:

pip install git+https://github.com/scottrbrtsn/open-dis-python@pyproject-poetry-init

pyproject.toml Show resolved Hide resolved
@leif81 leif81 requested a review from ricklentz March 8, 2024 12:14
@leif81 leif81 merged commit 9312cb2 into open-dis:master May 10, 2024
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.

Will this become a Pip package?
3 participants