-
-
Notifications
You must be signed in to change notification settings - Fork 372
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
Introduce package for Briefcase Automation #1549
Conversation
ab99d83
to
6eb07db
Compare
For posterity:
|
I'll pause here for comments on how I've incorporated the briefcase automation plugin in to the Briefcase repo. This approach of a dedicated top-level directory seemed appropriate given the options I could muster. For instance, we could use the Toga approach of nested pyprojects within Something I'd also like to explore is how to make an extra dependency group for this automation package; that may help simplify getting it installed in to CI. However, I'm not sure how a reference to this dependency could be reliable in all cases.... Another alternative altogether I've considered is not creating a new package....but to include these automation bootstraps within Briefcase directly....such that they would even be shipped with releases. However, they would not be exposed as entry points by default. Instead, they could be dynamically loaded at some point during testing. |
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.
A couple of relatively minor suggestions, but on the whole I like the approach. A standalone install in the same repo makes sense to me - this won't be useful to anyone outside of Briefcase, so I don't see any reason to include it in the main package.
Making it an optional install on Briefcase is an interesting idea... but I'm not sure how that would work without either publishing the package on PyPI, or installing via a GitHub reference. AFAIK, you wouldn't be able to reference the dependency as local files unless the code is included in the published package. But I guess the current setup in .github is already installing the automation package via GitHub, which would just mean the configuration can be self-contained in the Briefcase repo.
automation/pyproject.toml
Outdated
] | ||
dependencies = [ | ||
"briefcase >= 0.3.17.dev100", | ||
"toml", |
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.
If you've got a dependency on briefcase, you've got toml - either as tomllib
(in the Python 3.11+ standard library), or tomli
on earlier versions. No need to introduce another toml library.
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.
yeah...i was in the middle of trying to get this new package to work and then realized there's several TOML packages in play (toml
, tomllib
, tomli
, and tomli_w
) and didn't want to spend time then trying to understand it. Nonetheless, I've updated this to use the TOML packages that Briefcase is already using.
automation/pyproject.toml
Outdated
"Operating System :: OS Independent", | ||
] | ||
dependencies = [ | ||
"briefcase >= 0.3.17.dev100", |
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.
Looks like a perfect excuse to use setuptools_dynamic_version to me :-)
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.
Ok....this took me a minute. Your hyperlink returns a 404; so, I was frantically searching the internet for what on earth this is....and then I realized this all some brand new stuff for toga 🤣 Anyway, now that I understand, I got it working.
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.
hmm....my first attempt at this is hampered by GitHub's PR/CI tactics.
The version of Briefcase installed in to CI is briefcase-0.3.17.dev118+g169210e9
.
The version of Briefcase wanted by the Automation plugin is briefcase==0.3.17.dev117+g10ed6d70
.
I'm pretty certain this is stemming from GitHub creating a new merge commit for CI for the PR and that's what actions/checkout
is fetching....therefore, the Briefcase artifact is one commit ahead.
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.
hmm....this is non-trivial...but not impossible, of course. If we're going to put some kind of constraint on the version of Automation matching the version of Briefcase, then the two will always need to be sourced from the same place.
Given that Briefcase can be installed several different ways in to CI, each method will need to accommodate also installing Automation...or packaging Automation as well so it can be installed later.
Probably won't have time tonight but I'll return to this.
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.
Apologies for the run-around on this - for posterity, the link should have been to https://github.com/beeware/setuptools_dynamic_dependencies.
As for the version mismatch - I'm intrigued that this doesn't happen in Toga... I'm guessing the reason is that we're explicitly building wheels and installing those wheel, rather than installing from the repo directly.
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.
Scenarios for how Briefcase is installed
- CI event for beeware/briefcase PR
- Briefcase is packaged from CI's PR merge commit and wheel is installed
- Automation should be installed from CI's PR merge commit
- CI event for beeware/briefcase that is not a PR
- Briefcase is packaged for the current git ref and wheel is installed
- The most common situation for this is CI running after a PR is merged in to
main
- Although, the git ref could also be a tag such as when releases are cut
- The most common situation for this is CI running after a PR is merged in to
- Automation should be installed from the same git ref
- Briefcase is packaged for the current git ref and wheel is installed
- CI event for other repo
- Briefcase is installed from a git ref relevant for the event
- Automation should be installed for the same git ref
Strategy
So, while it is relatively straightforward to say "use the same git ref to install Automation"...the ref used to install Briefcase may not be readily available arbitrarily later in the CI run. So, it'll probably be easiest to package and/or install Automation at the points in the CI when Briefcase is packaged and installed.
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.
This strategy turned out not to be too invasive and actually pretty straightforward. There is a complication from running app-build-verify
for PRs for versioned branches of template repos where the automation plugin may not be available (or supported anyway).
automation/pyproject.toml
Outdated
|
||
[project] | ||
name = "briefcase-automation" | ||
version = "0.1.0" |
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.
Probably safest to make this a dynamic setuptools_scm version. Avoids any confusion over which version is installed/in use.
6fa2b60
to
035bbf7
Compare
2868c36
to
bee9054
Compare
bee9054
to
0813fba
Compare
This can be merged independently of the other changes; that'll help confirm Although, to be clear, I still need to update the CI in this repo before we actually merge. |
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.
One minor Q about the PPB automation, and the workflow CI references, and this otherwise looks good to go.
To be clear about the landing process, though - once the CI references are fixed, we can land this; but the effect will be that the automation bootstrap will exist, but won't be used. We then land the .github change, and that enables the use of the automation plugin on current/future Briefcase PRs. The current branch-based CI references exist to validate that the "end state" when we're using the plugin, the plugin will actually work.
Have I got that right?
|
||
def pyproject_table_linux_flatpak(self): | ||
table = tomllib.loads(super().pyproject_table_linux_flatpak()) | ||
table.setdefault("requires", []).append("pysdl2-dll==2.0.22") |
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.
Why is this only part of the automation, and not the core ppb bootstrap?
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 just because I wasn't sure if we wanted to officially endorse this for new projects....but I'm probably only really being conservative because I don't know anything about the ppb project.
That said, it seems fairly harmless to add this to the actual bootstrap.
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.
Probably worth tagging @pathunstrom to see if she has any thoughts on the matter.
/me turns on the bat signal...
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.
Took another quick look at this. I noticed that PPB only uses the pysdl2-dll
package for macOS and Windows; that implies to me that Linux system packages are expected to fill the gap on Linux.
Indeed, for at least Ubuntu 22.04 based distros, installing SDL2 only from the package manager works. However, this doesn't work for Flatpak since it can't find these system libraries. Therefore, it seems best, or at least most straightforward, to just continue using pysdl2-dll
for all platforms.
However, this means that SDL2 no longer needs to be installed in to CI with apt
to actually run PPB apps. I've removed this in beeware/.github#69.
Briefcase CI without installing SDL2 packages from apt: https://github.com/beeware/briefcase/actions/runs/7088969859
P.S. - additional future work could probably tweak some of this so Linux System package builds with Briefcase for PPB can depend on the distro packages for SDL2.
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.
Via Discord: @pathunstrom has indicated the patch is on the right track; we'll likely need to revisit it when PPB can address the problem at their end, but for now, it works.
Yep; that's spot on. |
50d44fe
to
5fa9caa
Compare
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'm OK with all of this; I'm holding off merging until we hear from @pathunstrom about the PPB dependency.
skip_install = True | ||
passenv = FORCE_COLOR | ||
deps = | ||
build==1.0.3 | ||
twine==4.0.2 | ||
commands = | ||
python -m build --outdir dist/ . | ||
python -m build . --outdir dist/ | ||
with-automation: python -m build automation/ --outdir dist/ |
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.
Actually - I take that back: the -with-automation target has been removed from the GitHub CI (it didn't show up in my earlier review because the CI workflow doesn't appear in the patch any more); but it doesn't appear to be used by the .github repos either. Am I missing something?
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 introduced the ability to pass additional tox factors to the python-package-create
workflow with beeware/.github#69. And passing unknown inputs to a variable causes CI to fail; so, I'll need to create another PR here that'll need to be merged with beeware/.github#69.
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.
Sorry - is #1460 the right reference there?
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.
my bad - too many tabs open - I meant beeware/.github#69
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 makes more sense :-)
So the change is required, but we need another step in the landing strategy to re-introduce the bit that builds the automation plugin wheel once both this PR and the .github PR lands - and that change will be the one that actually turns on the "enable automations" part of the testing strategy. Have I understood correctly? And there's no intermediate state where builds will fail that we need to just "land both fast"?
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.
Yes; mostly I think.
Getting the automation plugin in to main
will help simplify the continued testing of beeware/.github#69. Once this PR is merged, I'll be able to fix these failures for Briefcase in .github
's CI as well.
Then, I'll just need to create one final PR here that passes tox-factors
to python-package-create
workflow. This PR will be co-dependent on beeware/.github#69 and will need to be merged in at the same time.
dceb48b
to
408d6b1
Compare
- ppb==1.1.0 is not compatible with newer versions of pysdl2 libraries
408d6b1
to
fc487b2
Compare
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.
👍
Changes
app-build-verify
.github#69PR Checklist: