-
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
Run apps created in app-build-verify
#69
Conversation
b361bb8
to
e71d064
Compare
An interesting conundrum. If another repo, like Notably, consider Briefcase's CI calling This will require knowing the repo and branch where the running |
5d24435
to
1ae64c7
Compare
Another person pointed out this same issue. They did not really come to a reasonable conclusion, though...since their workable solution was a hacky javascript action that manually inspects the environment to derive where the running reusable workflow lives....and using some OIDC context at that. The core problem is mostly how inheritance works. When the reusable workflow (i.e. Relevant bits of
So, the reusable workflow being called effectively appears to just be a part of the workflow that called it. This means a naked There doesn't appear to be a straight-forward way for a reusable workflow on a non- Anyway, I suppose I'll add inputs to |
66d32b8
to
098cdb0
Compare
9c55cb8
to
ef90a31
Compare
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 questions for clarification purposes, and one minor cosmetic thing - but otherwise, I think this all makes sense, and it's difficult to argue with the success of the example cases you've provided.
I definitely can't see anything that is systematically wrong or will lock us out of a use case that we need to support; my only real concern is the level of complexity... but to some extent, this is just inherently complex.
if: > | ||
startsWith(inputs.runner-os, 'ubuntu') | ||
&& contains(fromJSON('["Linux"]'), inputs.target-platform) | ||
&& contains(fromJSON('["AppImage"]'), inputs.target-format) | ||
working-directory: ${{ steps.create.outputs.project-path }} | ||
run: | | ||
PACKAGES="libfuse2" | ||
case "$(tr '[:upper:]' '[:lower:]' <<< '${{ inputs.framework }}')" in | ||
toga ) PACKAGES="${PACKAGES} libthai-dev libegl1" ;; |
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.
libthai-dev? What's that for?
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....this goes back to all our previous AppImage testing; libthai-dev
is a deep transitive dependency for Toga but AppImage has excluded it from inclusion....so, this ensures it's available to load from the file system of the runner itself.
For completeness, here's the minimum apt
packages to run Toga in an AppImage:
libthai-dev libfribidi0 libfontconfig1 libx11-6 libegl1 libgl1 libharfbuzz0b
All the other packages are already installed.
.github/workflows/ci.yml
Outdated
@@ -229,8 +229,8 @@ jobs: | |||
strategy: | |||
fail-fast: false | |||
matrix: | |||
framework: [ "toga", "ppb" ] | |||
runner-os: [ "macos-latest", "windows-latest", "ubuntu-latest" ] | |||
framework: [ toga, pyside6 ] |
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.
Not sure I understand this change. Is Pyside a better check?
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.
Ah; this was a really early change I made when this was still POC. There isn't really a defensible rationale other than it took me a lot longer to get PPB working than PySide. I can change it back, though.
briefcase-template-source: ${{ inputs.briefcase-template-source || steps.template.outputs.path }} | ||
briefcase-template-branch: ${{ inputs.briefcase-template-branch }} | ||
# use the current repo checkout if briefcase-template is being tested | ||
briefcase-template-source: ${{ endsWith(github.repository, 'briefcase-template') && github.workspace || '' }} |
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 presume the drop of briefcase-template-branch
is because we can't support non-main branches any more?
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 is definitely a little more obscure; so, #68 added the ability to specify the briefcase template as part of the body of the PR; so, manually overriding it in the workflow isn't necessary anymore.
|
||
echo "Rolled out project to ${APP_PATH} (${{ inputs.framework }}->${GUI_INPUT})" | ||
echo "Rolled out project to ${APP_PATH} (${{ inputs.framework }}->${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.
A minor cosmetic thing, but it took me a couple of tries to work out how to "read" this when it appeared in the logs. It might help to be more explicit about the framework and bootstrap choice.`
30ab644
to
f043605
Compare
f043605
to
f52e45e
Compare
This is all here now. I'm not sure where we stand now with any further parameterization of Otherwise, the only other question is CI for backports for templates. I think we'll just need to abandon running CI for such PRs; I definitely do not see any low-effort option to keep it working. Although, once Briefcase 0.3.17 is cut, CI would be able to run for backports to that version and later. |
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.
All looks good to me.
Regarding the potential parameterisation: I think I'm happy for now. It isn't immediately obvious to me that parameterisation will result in something that is obviously better - I think we'd likely end up exchanging some complexity in this project for equal levels of complexity spread across all the templates (... or, straight up more complexity in both 😝).
We can always revisit this if one of us has a bright idea about how to simplify everything, but for now, the perfect is the enemy of the good, and what we've got definitely meets the bar of good (and then some).
I just realized that CI for this PR should be re-run without |
Changes
briefcase run
for apps created withapp-build-verify
app.py
with a version that will automatically exit shortly after the app startsNotes
briefcase run
is not called for apps created for Linux via containerization since Briefcase does not support the Run command when--target
is specified (and that's because passing the X11 into Docker is convoluted).PR Checklist: