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

Add GitHub Actions CI, modernize packaging #302

Merged
merged 11 commits into from Dec 16, 2023
Merged

Conversation

ferdnyc
Copy link
Contributor

@ferdnyc ferdnyc commented Dec 16, 2023

This PR makes several changes to the repo, and might be more easily reviewed commit-by-commit, though there are a lot of them so perhaps it's best to just go file-by-file.

  • Packaging is modernized

    • Python 3.7 as the minimum version
    • The bulk of metadata, and all dependency configs, moved from setup.py to pyproject.toml
    • Unsupported setup.py feature tests_require migrated to a new optional dependency set defined by the tests extra (pydot[tests])
    • Other extras defined in pyproject.toml:
      • The old dev extra, containing chardet and black
      • A release extra, which installs the zest.releaser tool along with its [recommended] addons
  • The pyparsing dependency now requires a version <3, due to the issues with pyparsing 3.0+. This allows pydot to install and run on all platforms. @lkk7's PR Fix multiple breaking issues from new pyparsing versions #296 should be updated, if this is merged first, to then also remove that restriction.

  • Testing is scaffolded

    • tox is now the testing framework, with its configuration defined in setup.cfg. (there's no tox.ini, another config file seemed unnecessary)
    • tox is configured with test environments for every Python version 3.7–3.12 inclusive (py37 to py312)
    • Black linting is also made part of the tox configuration, with an additional environment black defined to run the tool under Python 3.12
    • The means by which test/pydot_unittest.py discovers the path to its own directory is updated (see diff).
  • GitHub Actions CI is enabled (building on the work started in Add tox and github actions #299)

    • A separate test instance will be run for each Python 3.7 – 3.12
    • Currently tests are only run on ubuntu-latest platform, but MacOS and/or Windows can be added if we wish
    • Black linting is performed as part of, and only under, the Python 3.12 testing, in the same runner.
    • toxdev/tox-gh is used to integrate with the GitHub Actions matrix, and on each runner instance will run only the set of environments defined for the installed Python.
  • All of Black's changes to the source formatting are applied and checked in, so that the existing code passes its linting.

  • The Travis and AppVeyor configs are deleted. (They're in the gi t history if anyone ever needs them.)

...I think that's about it. Have at it! Any reviews/feedback welcome.

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Dec 16, 2023

A Successful run of the GH Actions CI can be seen here:

https://github.com/ferdnyc/pydot/actions/runs/7231448851

https://github.com/ferdnyc/pydot/actions/runs/7231618322

(With all of the subsequent commits applied.)

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Dec 16, 2023

RE: The GH Actions permissions, when running workflows in my fork I noticed that the GITHUB_TOKEN had full read/write permissions to the repo. I'm not sure if that config was defaulted generally, or copied from the upstream repo (here), but for security reasons it's advisable to lock it down.

It'd be helpful if someone with repo admin rights could go into Settings > Actions > General and change the "Workflow permissions" configuration (bottom of the page) from "Read and write permissions" to "Read repository contents and packages permissions", if it's not already selected.

Nothing the CI is doing requires repo-write permissions. The only reason that would need to change is if we decided to have Black automatically apply its own corrections.

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Dec 16, 2023

  • Unsupported setup.py feature tests_require migrated to a new optional dependency set defined by the tests extra (pydot[tests])

I'm not sure it's actually necessary to have chardet and black in there, since they need to be installed to the inner environments tox creates — and they're redundantly defined as dependencies in the setup.cfg toxenv sections for that reason. Installing them in the outer Python is actually unnecessary/pointless (from a testing perspective).

But, it makes pydot[tests] a superset of pydot[dev], and means users won't have to install both extras separately if they want the full testing/dev environment.

@lkk7
Copy link
Member

lkk7 commented Dec 16, 2023

@ferdnyc Thanks for this great work, I see that tests pass and there will probably be no reviewers as of now.
I'm ready to merge this if you don't plan on adding anything else

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Dec 16, 2023

@lkk7 Nope, good to go from my perspective! Only outstanding item is whether we want macOS/Windows CI as well.

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Dec 16, 2023

(Even if we do, that can be a followup PR.)

@lkk7
Copy link
Member

lkk7 commented Dec 16, 2023

The original CI had some Windows workflow, and it was something conda-related. I don't know conda that much to know if it's still relevant. That can be looked at in the future changes.

@lkk7 lkk7 merged commit 905116d into pydot:master Dec 16, 2023
@ferdnyc
Copy link
Contributor Author

ferdnyc commented Dec 16, 2023

@lkk7 I suspect Conda was a means of getting graphviz installed, but IMHO that's probably easier done with something like Chocolatey (which I know packages graphviz).

@ferdnyc ferdnyc deleted the gh-actions branch December 16, 2023 12:01
@ferdnyc
Copy link
Contributor Author

ferdnyc commented Dec 16, 2023

It's a party in pydot.

image

@lkk7
Copy link
Member

lkk7 commented Dec 16, 2023

Great, I'll look at my change and make sure it's OK. And update the pyparsing version

license = {file = "LICENSE"}
requires-python = ">= 3.7"
dependencies = [
'pyparsing>=2.1.4,<3'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lkk Yeah, you just need to drop the ,<3 here, in #296.

lkk7 added a commit that referenced this pull request Dec 16, 2023
Relevant discussions in: #296, #302 

* Fix broken parsing (whitespace problems and multilingual identifiers)

* Fix broken HTML parsing

* Update pyparsing to `>=3.0.9`

* Remove `parse_html` function

* Set pyparsing version to >=3 (anything below will break)

* Update the changelog, including the latest CI change

* Update formatting according to `black`
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

3 participants