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

Modify path handling to use dist and build folders #1107

Merged
merged 8 commits into from
Mar 8, 2023

Conversation

freakboy3742
Copy link
Member

@freakboy3742 freakboy3742 commented Feb 26, 2023

We've had a number of issues in the past related to the use of spaces in paths. Android NDK breaks if the path passed to gradle contains spaces or special characters (#696, #789); early versions of Flatpak break if the project path has spaces (#804). The process of building an RPM packaging backend has revealed that RPM has a similar bug - rpm–build doesn't support spaces in project paths.

Briefcase currently uses the formal name as part of the bundle path, which makes builds succeptible to these problems. We were able to address #789 by careful use of working directories; #696 by overwriting the bundle path for Android with a "safe" formal path; #804 can be fixed with a flatpak update. However, the bug in rpm-build known since at least 2006, so it seems unlikely it will ever be fixed; and there aren't any particularly viable workarounds (that I can see, anyway).

In short, using formal name in the bundle path seems to be more trouble than it is worth.

On top of that, there's a long standing feature request to simplify the location of output artefacts (#424); and a general problem that a working Briefcase project folder ends up with lots of working directories (macOS, linux, windows, web, android, iOS).

This PR addresses both of these issues. It changes the location in which projects are rolled out to just 2 folders: build and dist. Templates are rolled out into the build directory, separated as build/<app-name>_<version>/platform/format; by using the app-name, we're guaranteed that we won't have spaces in a Briefcase-provided path. All artefacts are dropped into the dist folder, so uploading dist/* is a reliable distribution strategy. It also means Briefcase now uses an artefact layout that is closer to the conventions seen with wheel packaging.

Other notable details:

  • The changes from Add a Linux system packaging backend #1106 removing app packaging format as an argument to distribution_path have been incorporated here, as there is significant crossover between the handling putting distribution artefacts into a single folder.
  • macOS app packages are compressed into a single .zip file (named appname-version.app.zip) for distribution. This means all distribution artefacts are guaranteed to be a single file.
  • The path passed to Docker containers is now the bundle path, rather than the base linux path, simplifying paths needed to invoke commands inside Docker.
  • The filename of web distributions is now appname-version.web.zip.

This requires a change to every template to change the top-level folder; cookiecutter requires that top level folder is templated, even though the name is fixed, so the cookiecutter.json for every template now defines a format variable. See:

Fixes #424
Fixes #804

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

@freakboy3742
Copy link
Member Author

freakboy3742 commented Feb 27, 2023

Bother... CI is stuck in a chicken-and-egg situation. I thought this would be avoided by the recent .github changes, but it looks like we missed a use case.

I thought that when I opened a PR against each template, if I used the same branch name, then the template CI would pick up and install this branch; so the CI on the template would pass, we could merge it, re-run CI on this PR, and it would pass.

However, the templates aren't picking up this branch for CI purposes (e.g. the macOS App template). My initial thought was that this might have been because the templates are branches on the main repo, but this PR is a branch on my fork. However, looking at the logs on the template CI failures, I don't think that was even tried as a strategy. @rmartin16 Have I misunderstood what should be happening here?

@rmartin16
Copy link
Member

hmm....we may have missed a design requirement of the beeware/.github consolidation.

In the install-briefcase github action, it does some waterfall logic to determine the appropriate source of Briefcase. However, that logic does not include evaluating whether git for Briefcase contains a branch named the same as the branch being tested in CI. It only considers if the target branch of the PR is also a tag in Briefcase's repo.

The install-briefcase action does already include a printout of the relevant refs in the CI:

Workflow Trigger Details
  Event:    pull_request
  Ref:      refs/pull/16/merge
  Base Ref: main
  Head Ref: build-dist
  Ref Name: 16/merge

So, the action could be updated to capture a CURRENT_VERSION from ${{ github.head_ref }} perhaps alongside the TARGET_VERSION for PRs and later check if that is ref in Briefcase.....if so, use it; if not, move on.

@rmartin16
Copy link
Member

However.....the branch for this PR isn't actually in beeware/briefcase.....so, finding branches like this from an arbitrary template repo may be a bit more non-trivial....

@freakboy3742
Copy link
Member Author

However.....the branch for this PR isn't actually in beeware/briefcase.....so, finding branches like this from an arbitrary template repo may be a bit more non-trivial....

Yeah - that seems like it could be the bigger issue. The only approach I can think that might work would be to use something like "Briefcase-1234" as the template branch name, so that there's a clear connection between template and briefcase versions.

@rmartin16
Copy link
Member

However.....the branch for this PR isn't actually in beeware/briefcase.....so, finding branches like this from an arbitrary template repo may be a bit more non-trivial....

Yeah - that seems like it could be the bigger issue. The only approach I can think that might work would be to use something like "Briefcase-1234" as the template branch name, so that there's a clear connection between template and briefcase versions.

I don't think I'm following this entirely...

FWIW, I dealt with this chicken/egg problem by manually specifying the Briefcase repo and branch as inputs for the action in the template repos. It's definitely tedious but does allow running CI against an arbitrary source of Briefcase.

@freakboy3742
Copy link
Member Author

However.....the branch for this PR isn't actually in beeware/briefcase.....so, finding branches like this from an arbitrary template repo may be a bit more non-trivial....

Yeah - that seems like it could be the bigger issue. The only approach I can think that might work would be to use something like "Briefcase-1234" as the template branch name, so that there's a clear connection between template and briefcase versions.

I don't think I'm following this entirely...

Using this PR as an example: if the branch on the template repo is "Briefcase-1107", rather than of "build-dist", it doesn't matter which repo the briefcase branch is on - you can use the PR number as the point of reference.

FWIW, I dealt with this chicken/egg problem by manually specifying the Briefcase repo and branch as inputs for the action in the template repos. It's definitely tedious but does allow running CI against an arbitrary source of Briefcase.

Unfortunately, that means modifying the CI configuration, but the change is only valid while the PR is in flight. It would work, but it would also need an cleanup PR afterwards to remove the customisation.

@mhsmith
Copy link
Member

mhsmith commented Feb 27, 2023

Templates are rolled out into the build directory, separated as build/<app-name>_<version>/platform/format

Why include the version here? Aside from cluttering an already-long path, doesn't that mean that changing the version will force an entirely new directory to be created, and a full rebuild of the app? It'll also waste space by accumulating old version directories which are unlikely ever to be useful.

Even if a re-create is the only way we currently have of updating the version on some platforms, the same is true of many other pyproject.toml settings, so I think this would be better addressed with a general solution (#472).

@freakboy3742
Copy link
Member Author

Templates are rolled out into the build directory, separated as build/<app-name>_<version>/platform/format

Why include the version here? Aside from cluttering an already-long path, doesn't that mean that changing the version will force an entirely new directory to be created, and a full rebuild of the app? It'll also waste space by accumulating old version directories which are unlikely ever to be useful.

Even if a re-create is the only way we currently have of updating the version on some platforms, the same is true of many other pyproject.toml settings, so I think this would be better addressed with a general solution (#472).

My general thinking here was to follow the pattern of setuptools, which include a version in their build directories. Setuptools also includes the build architecture - I considered going down that path and making the build directory a flat rather than nested hierarchy (i..e, something likebuild/myproject_0.0.1-linux.flatpak, rather than build/myproject-0.0.1/linux/flatpak) - but I thought the hierarchy was potentially useful, especially in the cleanup case (e.g., delete all linux builds, but preserve macOS builds) - although I guess you could also meet that requirement with wildcards.

More generally, including the version number also encourages thinking of the build folder as a transient artefact, rather than something that can be manually modified - or, if you do commit parts of the build folder, it means the modifications are specific to a particular build.

That said - this is very much in the "strong opinions, weakly held" basket - I won't fight especially hard to defend the version number or deep hierarchy decision. Any folder name will functionally work; I'm happy to change the implementation if you feel the version number or deep heirarchy is more trouble than it's worth.

@freakboy3742
Copy link
Member Author

freakboy3742 commented Mar 1, 2023

Thanks to @rmartin16's fixes to .github, we can now resolve the chicken/egg problem. The build-app tests on this PR are currently failing; however, the CI tests on each of the template branches are now running using this briefcase branch.

Once the template CI tests are apposing, those template PRs can be merged to their respective main branches, and the failing tests on this branch can be re-run. That should be a formality, and this PR can be merged.

(Side note: what we need is the ability to override the app settings at the command line, so that in CI we can say something like briefcase build --app-setting template-branch=build-dist. That would allow briefcase PRs to set a specific template branch. I've opened #1115 to track this)

@mhsmith
Copy link
Member

mhsmith commented Mar 6, 2023

I think the hierarchy is a good idea, and it has a nice correlation with the structure of the section titles in pyproject.toml. But I'd prefer to remove the version number. Even though we'd like there to be no need for the user to manually edit the build directory, it's still something we often advise people to do. The current rule is that those changes will remain in effect until the next time you run briefcase create, which is pretty easy to understand. But it would be a lot more confusing for the changes to spontaneously disappear when the user changes the app's version number. And even more confusing if they've updated Briefcase itself since the last briefcase create, because the new build directory – which they never intended to create – may be generated from a different template with even more changes in behavior.

Also, as discussed here, if we intend to switch the platform capitalization to lower-case, then we should make the directory names lower-case now. Otherwise we'll have to do extra work in the future to rename them a second time.

@freakboy3742
Copy link
Member Author

I think the hierarchy is a good idea, and it has a nice correlation with the structure of the section titles in pyproject.toml. But I'd prefer to remove the version number.

Sounds reasonable to me. It's gone.

Also, as discussed here, if we intend to switch the platform capitalization to lower-case, then we should make the directory names lower-case now. Otherwise we'll have to do extra work in the future to rename them a second time.

I initially thought that this would be extremely invasive - however, it occurred to me that we only need to change the representation in build-path to make the output folders consistently lower case, while retaining all the "preferred case" for human-visible command output, etc. I've added this in my most recent pass.

@mhsmith
Copy link
Member

mhsmith commented Mar 7, 2023

I won't merge this just now, because I don't know if it would be more convenient to merge #1106 or #1107 first.

@freakboy3742 freakboy3742 merged commit 4939993 into beeware:main Mar 8, 2023
@freakboy3742 freakboy3742 deleted the build-dist branch March 8, 2023 02:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants