Skip to content

Conversation

@echedey-ls
Copy link
Contributor

@echedey-ls echedey-ls commented Mar 18, 2025

This PR is my own agenda for Python Packaging, and an utility I plan to reuse as a pyOpenSci reviewer too. I based my work off pvplabs/pvpltools CI, which I also authored based on pvlib/pvlib-python (and a helping hand of AI agents too).

Main changes:

  1. Add a instructions header, please double check I didn't forget anything, it's been months since I last set up a CI like this
  2. Adds an easy-to-adapt env entry to change reused and subject-to-frequent changes options
  3. Former job has been split into two: one for building the distro files and one for publishing
    • building happens on all main pushes and on PRs that target main branch
    • publishing happens on main tags that start with v*

I wanted to keep the CI as similar to the prior version as I could. I failed. But feel free to make any objections, unify jobs, etc. It's all my design choices and how I feel it works the best, but that's just my opinion.

Can you confirm that this package uses PyPI's trusted publishing? I recommend doing a minor pre-release after merging, to ensure it all goes as expected.

EDIT: well I'll test this soon, will let u know.

@AdamRJensen
Copy link
Member

Can you confirm that this package uses PyPI's trusted publishing? I recommend doing a minor pre-release after merging, to ensure it all goes as expected.

@echedey-ls Yes, the package was switched to using trusted publishing in #60

@AdamRJensen
Copy link
Member

The docs failure seems to be caused by this PR.

@AdamRJensen
Copy link
Member

It seems sensible to me, but ideally, this needs a review from some one more competent. @kandersolar are you up for taking a look at this?

Copy link
Member

@kandersolar kandersolar left a comment

Choose a reason for hiding this comment

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

It looks reasonable to me as well. One question for @echedey-ls about GHA environments below.

# if this workflow is modified to be a generic CI workflow then
# add an if statement to the publish step so it only runs on tags.
# CI setup instructions:
# 1. Create a new environment for additional protection and security in the GitHub UI:
Copy link
Member

Choose a reason for hiding this comment

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

What's the benefit of using a GHA environment here? We won't be using any of the features (or will we?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know.
There's a claim in the setup instructions from PyPI that recommends it. I think they've clarified a bit why, I don't remember seeing it when I set it up.
In https://docs.pypi.org/trusted-publishers/creating-a-project-through-oidc/
Second image, the first one on setting up GHA:
imagen

And the note below it:

Like with "normal" Trusted Publishing, configuring a GitHub Actions environment is optional but strongly recommended.

@AdamRJensen I won't be able to test this locally with act until weekend/next week.

Copy link
Member

Choose a reason for hiding this comment

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

@echedey-ls let me know if I should do something, e.g., merge this PR and make a pre-release.

@AdamRJensen
Copy link
Member

Upon merging in #62 I got the following email detailing an error rin the PyPI workflow:

image

@echedey-ls
Copy link
Contributor Author

I will look into it later, please don't merge for now. I suspect it can be the trigger, that does not work with merge commits. I have to update the ci name too, so I'll commit that.

@echedey-ls
Copy link
Contributor Author

Good to go, I believe :)

@echedey-ls
Copy link
Contributor Author

There are some warnings in the build step you may want to check:

/tmp/build-env-uxvjhs_v/lib/python3.12/site-packages/setuptools/config/_apply_pyprojecttoml.py:82: SetuptoolsDeprecationWarning: `project.license` as a TOML table is deprecated
!!

        ********************************************************************************
        Please use a simple string containing a SPDX expression for `project.license`. You can also use `project.license-files`.

        By 2026-Feb-18, you need to update your project and remove deprecated calls
        or your builds will no longer be supported.

        See https://packaging.python.org/en/latest/guides/writing-pyproject-toml/#license for details.
        ********************************************************************************

!!
  corresp(dist, value, root_dir)
/tmp/build-env-uxvjhs_v/lib/python3.12/site-packages/setuptools/config/_apply_pyprojecttoml.py:61: SetuptoolsDeprecationWarning: License classifiers are deprecated.
!!

        ********************************************************************************
        Please consider removing the following classifiers in favor of a SPDX license expression:

        License :: OSI Approved :: BSD License

        See https://packaging.python.org/en/latest/guides/writing-pyproject-toml/#license for details.
        ********************************************************************************

https://github.com/pvlib/twoaxistracking/actions/runs/13958847092/job/39076147809?pr=64

I fixed some own-implementation errors and updated instructions (cause I removed base repo env var, it wasn't serving it's purpose ideally)

@AdamRJensen AdamRJensen merged commit 7f3c361 into pvlib:main Mar 21, 2025
19 checks passed
@AdamRJensen
Copy link
Member

I made a pre-release v0.2.7-alpha.4 and it works like a charm (don't ask about the pre-release naming 😄)

The files on PyPI are nice and small 12 kB.

Thanks @echedey-ls for this nice improvement!

@echedey-ls echedey-ls deleted the ci-build-n-publish-improvements branch March 21, 2025 09:41
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.

3 participants