-
Notifications
You must be signed in to change notification settings - Fork 43
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
Pin Jinja2 and MarkupSafe to fix issues building on Focal #211
base: master
Are you sure you want to change the base?
Conversation
Built charm on Jammy - https://paste.ubuntu.com/p/tP9X63bdWX/ |
@ajkavanagh , free to take another look at this PR? |
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.
Thanks again for your work on the patch. I am a bit concerned with the re-pinning of various modules as this will change the builds on existing stable charms (should they be rebuilt), which would be a significant change in behaviour of the charm. e.g. the unpins of setuptools and setuptool-scm seems particularly problematic.
I wonder if, tactically, you just override these in your charm's wheelhouse.txt?
@ajkavanagh different problem actually. The one @fnordahl & @darkalia saw and ran into was the Juju bug where it was selecting the charm with the highest revno and not one that was built for the specific series that they wanted. |
Just to be clear, I've seen no issues fixed by this proposed change, and I repeat my stance that fine grained pinning does not belong in layer-basic. The wide range of different approaches to reactive charming within the teams we know about (not to mention those we don't know about) makes this impractical. If you have specific pinning needs, doing so in individual charms or a different layer shared among a fleet of charms would be the preferred approach. |
The first issue is that charms fail to build on Focal. Here's quick steps I just came up with to reproduce: From a clean Focal VM or LXD container:
Now edit
Then create a
Now try build it with:
Unfortunately, it would fail with
So we'll want to pin Jinja2, create the
Re-try build. That should build successfully with the bundled wheelhouse Python packages:
The other issue is that some of our charms use Edit
Build charm:
That should build successfully with the bundled wheelhouse Python packages:
Now using that newly built charm, try to deploy to Focal:
It fails with:
|
@fnordahl I think you're right, I think going forward, for charms using |
@ajkavanagh, @fnordahl, okay PR updated to just resolve the build issues on/with Focal. |
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.
Okay, I'm generally happy with this as it's fairly minimal and does allow a minimal charm to have sensible defaults for Jinja2/markupsafe on Focal. As @fnordahl has some key thoughts on this, I'd also like his +1 on this. Thanks again for your work on this.
Happy to see a more focused proposal. Not going to block this change, but here are my further comments:
|
I think this is a valid critique and should be addressed. @hloeung could you update the PR to use "<" pins which would allow security fixes, please? With charms.reactive, charm-tools, layer-basic and the rest of the charms.reactive charm building system being in maintenance mode effectively, we don't want to do invasive or breaking changes in these tools. This means being very conservative about changes and ensuring that existing work flows continue to work (as best we can) but recognising that there is a better way of building charms now. |
Updated the PR to use "<" pins as advised. |
Trying to get this moving along... @ajkavanagh ? |
@hloeung juju/charm-tools#650 has merged now, try using type: charm
parts:
charm:
source: src/
plugin: reactive
reactive-charm-build-arguments:
- --upgrade-buildvenv-core-deps
build-snaps:
- charm/3.x/edge The new version will of course be promoted to stable, but most likely not until next year. |
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'm approving this as Jinja is pinned by version and MarkupSafe is a dependency of that; either we unpin everything or we continue to hold things per python version as this review is doing.
See #210