Issue 1202 - Make wizard work with existing projects

[yngvem]

Fixes #1202 (co-authored with @MarieRoald)

To make the wizard work with existing projects, we have added a --target option to briefcase new. The help text of this option states

  --target TARGET       The root path of the briefcase project. If the path
                        exists, then the wizard assumes that it contains an
                        existing project and will attempt to set up briefcase
                        with it (by merging the pyproject.toml files).
                        Otherwise, a new Briefcase project will be created at
                        the specified path.

If --target is specified, the wizard will run in a temporary directory. Then, dependent on whether the target directory exists, it will either merge the pyproject.toml files or copy all files generated by the wizard.

When the wizard is run and --target points to an existing directory, the user is additionally asked for the source directory and test source directory. The source directory must contain a __main__.py file, and the test source directory must exist (but may be empty). Furthermore, if the target directory is missing either a LICENSE file or a CONTRIBUTING file, then the wizard will show a warning message.

One important point is that for this change to work, we also need to change the template (see this PR for that).

PR Checklist:

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

[yngvem]

@freakboy3742, @mhsmith we believe this is ready for review now :)

[yngvem]

We'd be happy to work on it, but our schedule is unfortunately quite busy until the first week of June (ish). If you want to get this merged before mid June, then it's probably best if you continue yourself, otherwise we'll get back to it in June.

[freakboy3742]

We'd be happy to work on it, but our schedule is unfortunately quite busy until the first week of June (ish). If you want to get this merged before mid June, then it's probably best if you continue yourself, otherwise we'll get back to it in June.

Totally understood - I don't think this is super urgent, so if you're interested in continuing, we can wait until you're available again in ~June.

[MarieRoald]

We made an attempt at adding a new convert-command to briefcase (as discussed here). Currently, it will read PEP621-formatted metadata to try and help the wizard, but with options to overrule the inferred values whenever needed.

We plan to rebase on top of main soon, add tests, and revert the changes to briefcase new, but we wanted to check a couple of things with you first:

1. Do you think this new layout makes sense? You can run it yourself by running briefcase convert --template=gh:yngvem/briefcase-template --template-branch=explicit-source-dir-field
2. We still need a way to copy dependencies automatically - does Briefcase support standard PEP508-formatted dependency strings (e.g. package_name > 1.2.1; os_name != 'nt')? In that case, we could copy the dependency list from the [project] field in the pyproject.toml file into [tool.briefcase.app.{{app_name}}].

[mhsmith]

Not at present. It supports the version specifier part, but not the "qualifier" part (including and after the semicolon). Supporting PEP508 environment specifiers is arguably something we should support; however, we need to be careful how those specifiers and the platform-specific sections of the [tool.briefcase] configuration interact.

We might never have directly tested the environment markers, but it looks to me like they work fine. Are you aware of any problems with them?

[freakboy3742]

Not at present. It supports the version specifier part, but not the "qualifier" part (including and after the semicolon). Supporting PEP508 environment specifiers is arguably something we should support; however, we need to be careful how those specifiers and the platform-specific sections of the [tool.briefcase] configuration interact.

We might never have directly tested the environment markers, but it looks to me like they work fine. Are you aware of any problems with them?

Not from experience. I was assuming the syntax wasn't legal as a command line option to pip - but if they are legal, then I guess that simplifies things.

[MarieRoald]

We finally managed to pick this up again. Quite a few changes since last time (commit 0a93c1f8f8c3b35697ef007b48e90914e657906d).

• We rebased ontop of main
• We updated the pyproject merging so fields that fully overlap with what's in the pyproject.toml are removed (we based this on the briefcase.config.merge_pep621_config function)
• We inherit from NewCommand, which simplifies the code substantially
• We renamed ConvertCommand.new_app to ConvertCommand.convert_app
• We reset all changes to NewCommand so it matches what's in main exactly.

There are still some things to do, mainly making tests, but we wanted to run the code by you to see what you think of these changes before we spend much time migrating the unit tests to this new code layout.

We also need to reconsider how we handle the source directory and test source directory. We could either update the cookiecutter template and the NewCommand.build_app_context to set it without asking (like we had in 0a93c1f8f8c3b35697ef007b48e90914e657906d), or we can modify the output pyproject.toml file when we merge them. Do you have a preference there?

[yngvem]

We have tried to answer most of your questions and added questions to the ones where we were a bit unsure of how to proceed. One more question though, we noticed the project_overrides in the new command. Should we add that to the convert command as well, or is that unnecessary?

[freakboy3742]

We have tried to answer most of your questions and added questions to the ones where we were a bit unsure of how to proceed. One more question though, we noticed the project_overrides in the new command. Should we add that to the convert command as well, or is that unnecessary?

For consistency, it should probably be included.

[yngvem]

We have just pushed a version with tests where we tried to address all the comments so far (we're still missing docstrings for some tests, but I think the code is ready for a new review now).

You can run briefcase convert --template http://github.com/yngvem/briefcase-template --template-branch convert-command -Q author="Some Author" -Q unused=override to test it in action.

[MarieRoald]

We have tried to address all your comments, and while testing our implementation, we also noticed that the module name must be based on the app name, so there is a lot less flexibility in the source directory than we first thought. We updated the get_source_dir_hint-method to raise an error if there are no valid source directories based on the provided app name. Does that sound like a good solution?

[freakboy3742]

Also - don't worry about the ReadTheDocs test failure; that's a known issue that was resolved in #1695. If you merge main, you'll get a fix for that problem.

[freakboy3742]

@MarieRoald @yngvem Let me know when you're ready for a review; I can see this is passing CI, but I'm not sure if you've made all the changes you're expecting in this pass.

[yngvem]

There are still some changes left, but it should be ready for review again soon :)

[yngvem]

A lot of comments here - but we're definitely getting close now! The issues I've picked up are mostly nitpick style things, or minor edge cases and cleanups.

Regarding get_source_dir_hint() - you're correct that the module name is very tightly constrained; I think the approach you've taken is a good compromise (allowing for the edge case that I've highlighted inline). It's better to fail with a clear error than to try and be smart and generate a project that won't work. I can imagine a world where we dig into configurations for common packaging libraries like setuptools to introspect source directory configurations and entry points... but honestly, if a user has gone so far off base that their source directory isn't PEP621 name in the project root or src, then I don't hold much hope of their app working out of the box with Briefcase anyway :-)

The last missing piece is documentation. As we're adding a new command, we need a section in the docs (docs/reference/commands/convert.rst) to describe how to drive that command. Don't get too hung up on the specific language - a good first draft that covers all the major details is all we really need. Given how similar convert is to new, you can probably get 80% of the way by duplicating the new.rst in that folder and updating as appropriate.

We have added a draft for the documentation now so it should be ready for a review

[MarieRoald]

What a great message to start the day with! We're coming to PyCon US this year too, so we'll probably see you there 😄