-
-
Notifications
You must be signed in to change notification settings - Fork 397
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
Conversation
7bb78aa
to
5b3f5c3
Compare
"docker", | ||
"buildx", | ||
"build", | ||
"--load", | ||
"--progress", |
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.
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.
5b3f5c3
to
b09da47
Compare
6290a43
to
f03ebd0
Compare
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", |
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.
No objection to doing this bump if it's needed, but I'm interested what bug/feature has required it?
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.
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
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.
We should hold off. There's all kinds on new newlines in pyproject.toml
with 2.6.0.
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.
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.
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.
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.
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.
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 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.
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.
Good deal; I agree...especially since the issue is more widespread than I initially thought.
Another question that came up for me while I was implementing this: Do you see value in keeping 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 |
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. |
f03ebd0
to
30e5fa4
Compare
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. |
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.
👍
Changes
docker build
PR Checklist: