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 to pass arguments through to Docker image build #1661

Merged
merged 2 commits into from Feb 23, 2024

Conversation

rmartin16
Copy link
Member

@rmartin16 rmartin16 commented Feb 21, 2024

Changes

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

Comment on lines 909 to 913
"docker",
"buildx",
"build",
"--load",
"--progress",
Copy link
Member Author

Choose a reason for hiding this comment

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

Added buildx so it is explicit that buildx is building the image. Added --load because it is only implied when using the default build container; if you use a custom one, the image isn't loaded in to the local Docker image cache.

pyproject.toml Outdated
@@ -79,7 +79,7 @@ dependencies = [
# cause API breakages). If the package uses calver, we don't pin the upper
# version, as the upper version provides no basis for determining API
# stability.
"cookiecutter >= 2.3.1, < 3.0",
"cookiecutter >= 2.6.0, < 3.0",
Copy link
Member

Choose a reason for hiding this comment

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

No objection to doing this bump if it's needed, but I'm interested what bug/feature has required it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Using pre-2.6.0 and 2.6.0 produces two different files for app.py and __main__.py. I didn't track down the exact thing that changed but 2.6.0 introduced a way to control the jinja tag boundaries....so, thinking about it now, maybe a regression was introduced around how {% %} is replaced in a template....I should probably check how our other templates look now.

At any rate, CI was failing with 2.6.0: https://github.com/beeware/briefcase/actions/runs/7993409808/job/21829525533

Copy link
Member Author

Choose a reason for hiding this comment

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

We should hold off. There's all kinds on new newlines in pyproject.toml with 2.6.0.

Copy link
Member Author

Choose a reason for hiding this comment

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

well, actually, i guess it doesn't matter....2.6.0 is going to be installed regardless...so, I guess we may as well move forward and resolve this regression independently.

Copy link
Member Author

@rmartin16 rmartin16 Feb 22, 2024

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

To explicitly lay it out, I think these are the options I see to move forward:

  • Bump to require 2.6.0 as I have here
    • This would accommodate the whitespace changes in 2.6.0
    • Once/if these are resolved in cookiecutter, we can revert the changes and bump to that new version
  • Lower the upper bound from <3.0 to <2.6.0
    • This will obviously maintain the status quo
    • Once/if the issue is resolved, we can increase the upper bound again

Let me know what you think. As a note, CI for all PRs for Briefcase will fail until this is addressed.

Copy link
Member

Choose a reason for hiding this comment

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

I guess a third option is "!=2.6.0", if we have reason to believe that 2.6.0 is an isolated buggy release and the issue will be fixed in 2.6.1.

My inclination for now is to use "< 2.6.0" until we know whether this is an intentional change (or, at least, a change that won't be rolled back), or a bug that will be resolved. If this is the "new normal", then we should set the 2.6.0 as the new minimum; if it's a bug that gets fixed, we can exclude the specific problem releases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good deal; I agree...especially since the issue is more widespread than I initially thought.

@rmartin16
Copy link
Member Author

Another question that came up for me while I was implementing this:

Do you see value in keeping LinuxSystemPassiveMixin and LinuxSystemMostlyPassiveMixin as separate classes still given that LinuxSystemRunCommand now depends on the build system?

Some of the separation is already a little fuzzy to me....but I was also considering collapsing some of these forks in the code for whether the build system is the local machine or in Docker and just using the same logic for both via calls to tools[app].app_context.run().

@freakboy3742
Copy link
Member

Do you see value in keeping LinuxSystemPassiveMixin and LinuxSystemMostlyPassiveMixin as separate classes still given that LinuxSystemRunCommand now depends on the build system?

You're not alone in having this thought. I can't remember which PR I was working on where this case up, but I noticed the same thing. It was an optimisation in the past, but I'm fairly certain the use case for "mostly passive" no longer exists. I didn't get quite as far as doing the refactor, but if we can get rid of the "Mostly", I'd be happy to sign off on that.

@rmartin16
Copy link
Member Author

CI looks good now 🦾 once this is merged, I will re-run CI for beeware/.github#99 and re-base #1649; both of those should then be ready to review/merge.

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 aab9c67 into beeware:main Feb 23, 2024
51 checks passed
@rmartin16 rmartin16 deleted the extra-docker-build-args branch February 23, 2024 01:14
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.

None yet

2 participants