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

Create python wheel #915

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

hurricanehrndz
Copy link

@hurricanehrndz hurricanehrndz commented Nov 14, 2023

Make it possible to produce a python wheel form this project.

Benefits:

  • improved installation experience on non Darwin platforms
    pip install git+https://github.com/autopkg/autopkg.git@dev
  • adheres more closely to PEP-338
  • improved developer experience for testing:
    python -mvenv venv
    source venv/bin/activate
    pip install -r gh_actions_requirements.txt
    python -m build
    pip install --force dist/*.whl
    rehash
    autopkg version

Known deficiencies:

  • version number in pyproject.toml is hardcoded
  • wheel more than likely will not be accepted into python.org pypi server due to packages in this repo all being top-level

Autopkg currently delivers the autopkg python module as a console
script. This makes it near impossible to package the project into a
wheel. This PR intends to do the following:

- maintain backwards compatibility be creating a console script
  as close to as setuptools would
- move autopkg module into the autopkgcli
- fix test that failed due to move

see:
https://stackoverflow.com/questions/9133526/git-recover-failed-commits-message
https://peps.python.org/pep-0338/
[project]
name = "autopkg"
authors = []
version = "2.7.3"
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be sourced from another file? I know it is technically possible, but would be nice if the version was single sourced.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like AutoPkg does not have the current version of itself in the repo in a file, only in the git tags / releases?

Copy link
Author

Choose a reason for hiding this comment

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

Version is in plist which is incompatible with setuptools

Copy link
Contributor

Choose a reason for hiding this comment

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

where is the plist that contains the version? I was looking for it.

autopkg = "autopkgcli.autopkg:main"

[tool.setuptools.dynamic]
dependencies = {file = ["gh_actions_requirements.txt"]}
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with this, the other dependency files have too much stuff in them.

Copy link
Author

Choose a reason for hiding this comment

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

There is a lot in the current requirements that could be qualified as dev tools and not actually needed for the running of app. This should be trimmed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do agree, it makes sense to make a distinction between requirements for run/build vs requirements for dev/test/validation. I do think there is too much stuff in some of the requirements files for the sake of doing the basics in a github action or just running recipes on a build server.

I think the reason for the additional items in the requirements is because the requirements were traditionally only ever used by people doing dev work, while the production way to use autopkg was to install the PKG file on MacOS.

@jgstew
Copy link
Contributor

jgstew commented Nov 28, 2023

The biggest change this incurs is moving the main code from autopkg to autopkgcli/autopkg.py which is required to make this work best, but also a potentially breaking change that I don't personally know how to test sufficiently on MacOS since that isn't my area.

import plistlib
import sys
import unittest
from textwrap import dedent
from unittest.mock import mock_open, patch

import autopkgcli.autopkg # noqa: F401
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will have an issue if you are trying to dev/test autopkg source, then this will actually load autopkg from the pip installed version, not from the source version you are currently testing as a cloned repo.

I think this should only be a problem if you have previously pip installed autopkg? or maybe it only works if you have pip installed autopkg and won't work otherwise? I don't remember exactly.

autopkg = imp.load_source(
"autopkg", os.path.join(os.path.dirname(__file__), "..", "autopkg")
)
from autopkgcli import autopkg
Copy link
Contributor

Choose a reason for hiding this comment

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

Same issue: I think this will have an issue if you are trying to dev/test autopkg source, then this will actually load autopkg from the pip installed version, not from the source version you are currently testing as a cloned repo.

I think this should only be a problem if you have previously pip installed autopkg? or maybe it only works if you have pip installed autopkg and won't work otherwise? I don't remember exactly.

Copy link
Author

Choose a reason for hiding this comment

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

Not if you start using env python as your dev python

Copy link
Author

Choose a reason for hiding this comment

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

If this get merged, additional PRs will obviously need to be done to update docs on install and dev environment setup.

Copy link
Contributor

Choose a reason for hiding this comment

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

well I don't think using this method is required especially for the default use case on MacOS in which case the PKG would still be preferred.

@jgstew
Copy link
Contributor

jgstew commented Nov 28, 2023

@nmcspadden I think you have to approve this PR to run the github action automated tests.

@jgstew
Copy link
Contributor

jgstew commented Nov 28, 2023

The unit test failed due to:

github.GithubException.RateLimitExceededException: 403 {"message": "API rate limit exceeded for 13.105.49.65. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)"

it seems like the github token isn't being read from the default location if no default location is specified, which then causes this issue. This is due to a different bug.

See here: #899

@jgstew
Copy link
Contributor

jgstew commented Nov 28, 2023

I do think the fact that this change moves the core autopkg code from one file to another is a pretty significant change, and one that might not make sense at the moment with the potential impact it could have.

I think this would be an impediment to getting autopkg 3.x out the door as the main release version, which I think is higher priority than the potential problems this change could introduce at the moment.

I do like this concept in general though and do think it is worth future consideration.

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