-
Notifications
You must be signed in to change notification settings - Fork 4
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
Centralize More Universal CI Actions #8
Conversation
8f40a7e
to
9ea0e60
Compare
@freakboy3742, this should be ready for review now. I've got it working everywhere I think we need to use it. Also, these scripts kinda grew organically as I was finding requirements for each repo....so fresh eyes may see the forest i can't see through all the trees right now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woof... that's a lot of changes and dependent PRs :-)
I've flagged a few questions and comments inline; many of them are clarifications for my own benefit to make sure I fully understand what is going on; but broadly speaking, this is going in the right direction (and should make our lives a whole lot easier... once we land the changes 😅).
I haven't fully reviewed all the downstream repos yet - I've been working on the theory that we need to get this one right first, then get the other repos to be consistent with whatever lands. So - my comments here are mostly about the API surface and operation, guided by what I can see as the pre-existing CI usage, and intended usage once this PR is in place.
That said - a couple of things that I noticed:
- In the downstream repos (esp. Briefcase and Toga), I can see you've updated the CI target, but not the package target; that will be pulling artefacts from the CI build, so it will be impacted by the name change on the package workflow.
- I can't see anywhere in the Briefcase config that hits the Windows store python builds - am I missing something?
.github/workflows/towncrier-run.yml
Outdated
with: | ||
python-version: ${{ inputs.python-version }} | ||
cache: 'pip' | ||
cache-dependency-path: '**/setup.cfg' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we might want to add pyproject.toml
and setup.py
to this list as well. We're still using setup.py
in toga; I imagine that a move to full pyproject.toml
format in the future is inevitable (either as part of a setuptools upgrade, or a move to hatch), so including those files would seem like a good protective mechanism.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added **/pyproject.toml
as a path.
.github/workflows/package-python.yml
Outdated
with: | ||
python-version: ${{ inputs.python-version }} | ||
cache: 'pip' | ||
cache-dependency-path: '**/setup.cfg' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above - do we want to add pyproject.toml
and setup.py
to this list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So....this starts to get a little tricky. The setup-python
action hard errors if the files specified here don't exist. I'll give it a go and see if all the repos have these files.
(On a related note, this is why some of the other uses of setup-python
don't/can't use this built in caching.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can save you the trouble; pyproject should exist everywhere (and if it doesn't, that's a bug); setup.py currently does, but should be slowly disappearing. (I've deleted it in the deb PR branch, for example).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh - and the template repositories won't have any of these files - but then, we won't be building Python packages for those repos.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests finished after I added pyproject.toml
alongside setup.cfg
.
.github/workflows/package-python.yml
Outdated
@@ -0,0 +1,80 @@ | |||
name: Create Python Package |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "package-python" name is a little misleading... at the very least, it's "Python-package"; but if it's a beeware project, it's going to be Python regardless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's definitely fair.
With that, GitHub currently has a limitation requiring all workflows to exist in the .github/workflows/
directory. So, while we may not end up with too many workflows, I was looking for a way for workflows related to the same thing/tool/concept to sort well alphabetically. That's why I started the workflows for pre-commit
with pre-commit
and same with towncrier
. This leads, though, to a bit of awkward grammar with the action word coming last.
With that in mind, perhaps package-beeware-create.yml
best follows those rules (although equally awkward)....and perhaps where a workflow like package-beeware-deploy.yml
could theoretically exist alongside it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Landed on python-package-create.yml
.
|
||
inputs: | ||
briefcase-url: | ||
description: 'URL to Briefcase git repository.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've been trying to converge on " rather than ' in config files (for broad consistency with black); is there a particular reason you've reverted a bunch of those changes and moved to '?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly because "
and '
have different functional meanings in YAML....while in Python they are functionally equivalent.
Double quoted strings in YAML pass through a level of escape parsing that single quotes strings do not. So, really, I'm trying to emphasize that I literally want the value between the two single quotes without any interpretation.
That said, none of this is particularly consistent. For instance, all the places that GitHub is substituting values for ${{ github.event_name }}
(and the like), practically none of those are quoted at all.
So, unless this is compelling, I'm fine converging on double quotes. I also just realized that I was confusing TOML with YAML; TOML is a bit more restrictive about double quotes...
run: python -m pip install dist/briefcase-*.whl | ||
|
||
- name: Install Briefcase | ||
if: endsWith(inputs.repository, 'briefcase') != true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this not just !endWith...
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definitely is....my brain didn't want to believe !
was supported or something...
Updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interestingly....GitHub allows endsWith(inputs.repository, 'briefcase') != true
but requires the brackets for ${{ !endsWith(inputs.repository, 'briefcase') }}
....hmm...
97893cc
to
4b8d858
Compare
This is definitely the approach I intend.
I think you're talking about, e.g. the Reviewing
There's still the extra
|
I removed the testing code last night that was allowing - name: Checkout beeware/.github
uses: actions/checkout@v3.3.0
with:
repository: "beeware/.github"
path: "beeware-.github" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I picked up a couple of minor whitespace issues which I've pushed; but otherwise, I think this is good to go.
ohh. interesting....well, I've added |
I haven't merged yet - so if there's something else you want to squeeze in here, I can hold off... |
All right. Added the same basic |
New CI Workflows
pre-commit-run
pre-commit
checks for an arbitrary repo.pre-commit-source
defaults to.[dev]
; usepre-commit
if the repo doesn't define a pinned version.towncrier-run
tox -e towncrier-check
for an arbitrary repo.tox-source
defaults to.[dev]
; usetox
if the repo doesn't define a pinned version.package-python
tox -e package
and uploads as an artifact named 'packages-<repo name>'.verify-app-build
runner-os
andframework
are required at minimum.target-platform
andtarget-format
are not explicitly define, then all apps compatible with therunner-os
andframework
are built.install-briefcase
main
branch in a repo, Briefcase should be installed from itsmain
branch.v0.3.11
branch: https://github.com/rmartin16/briefcase-windows-app-template-test-1/actions/runs/4087055913/jobs/7049392731v10.0.0
branch: https://github.com/rmartin16/briefcase-windows-app-template-test-1/actions/runs/4087072170/jobs/7049409704v40.10.6
tag: https://github.com/rmartin16/briefcase-windows-app-template-test-1/actions/runs/4087695409/jobs/7049424029v0.3.12
tag: https://github.com/rmartin16/briefcase-windows-app-template-test-1/actions/runs/4087711584/jobs/7049438896v0.3.12
branch, thev0.3.12
version of Briefcase should be installed.main
branch: https://github.com/rmartin16/briefcase-windows-app-template-test-1/actions/runs/4086946506/jobs/7049469167v0.3.11
branch: https://github.com/rmartin16/briefcase-windows-app-template-test-1/actions/runs/4086976986/jobs/7049493706v10.0.0
branch: https://github.com/rmartin16/briefcase-windows-app-template-test-1/actions/runs/4087022947/jobs/7049517402main
branch is used when the version is unknown.ci
worklfow for.github
Implementation PRs
PR Checklist: