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 support for briefcase run to examples #1995

Merged
merged 6 commits into from
Jun 22, 2023

Conversation

rmartin16
Copy link
Member

@rmartin16 rmartin16 commented Jun 19, 2023

Changes

  • Allows briefcase run to work for the examples for as many platforms and output formats as reasonable
  • Given that these examples are currently the de facto method for users to see a widget in a native window, they should probably be runnable via briefcase run since python -m and briefcase dev aren't entirely native.

NOTE: This is easiest to review each commit individually.

Limitations

  • Linux
    • System
      • Running without a --target implies all the necessary packages are already installed on the host system
        • Maybe there's a way to help users be informed about this proactively....
      • Alternatively, supporting --target would require adding the appropriate system_requires to all the pyproject.toml and that just seems like a lot to maintain
    • AppImage & Flatpak
      • Both of these would require version-specific configuration in all the pyproject.toml.
      • While a relatively trivial sed command could be used to update these, the use-case for building these formats for these examples is weak

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

- Use `ThreadingHTTPServer` since it has much better support on Windows
  than `HTTPServer`; this was also experienced when web support was
  added to Briefcase
@rmartin16 rmartin16 marked this pull request as ready for review June 21, 2023 20:40
Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

Looks like you're working on another badge for your scout blanket :-)

Very thorough - nice catch on the handful of cleanups. One question on the postitron-static example, and a request for non-empty changelog/license files ,and I think this is good to go.

Copy link
Member

Choose a reason for hiding this comment

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

Empty files are a bit weird; it would be desirable to have at least some dummy content:

See Toga releases for change notes.

Copy link
Member

Choose a reason for hiding this comment

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

LICENSE files definitely can't be empty - I'm sure someone would choose to interpret this as "this code is free for all", and decide that applies to the entire Toga repo.

Released under the same license as Toga. See the root of the Toga repository for details.

@@ -9,7 +9,7 @@ def translate_path(self, path):
return str(self.server.base_path / path[1:])


class LocalHTTPServer(HTTPServer):
class LocalHTTPServer(ThreadingHTTPServer):
Copy link
Member

Choose a reason for hiding this comment

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

Was this a required change, or just one that seemed like a good idea?

Copy link
Member Author

Choose a reason for hiding this comment

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

The commit has a bit of this information but this was behaving strangely on Windows. The first few times I tried to start it, it would log "Starting server..." but not start serving the static page. After messing around with it for a bit, it seemed that after waiting a while, it would start working. But even then, it would often hang when trying to shutdown the server.

This was a lot of what we encountered when we added the web server for briefcase run web. Basically, it was a mess on Windows until we started using the ThreadingHTTPServer.

Copy link
Member

Choose a reason for hiding this comment

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

Ah - I missed that the explanatory detail was in the commit message.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

👍

@freakboy3742 freakboy3742 merged commit c3113be into beeware:main Jun 22, 2023
42 checks passed
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.

2 participants