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

wheelhouse: cleanup #206

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fnordahl
Copy link
Member

@fnordahl fnordahl commented Sep 8, 2022

With juju/charm-tools#620 and #205 in place, along with the Launchpad build- and Charmhub distribution- infrastructure being widely available, it is time to stop attempting to build wheelhouses that can install on any series and architectures.

The perferred method moving forward is to build a binary version of the charm per series and architecture pair and let the Python package manager do its job, releiving the charm author from the manual build dependency resolution chore.

Remove various build dependency workarounds from here as they do not apply to all charms.

Replace all pins with a less than next major version pin as the referenced issues have been resolved.

For any charm author that want to continue to use the old approach they are free to add build dependencies to their wheelhouses.

For older stable branches we recommend people to use the lock file feature [0][1].

0: juju/charm-tools#585
1: juju/charm-tools#598

Signed-off-by: Frode Nordahl [email protected]

With juju/charm-tools#620 and canonical#205 in
place, along with the Launchpad build- and Charmhub distribution-
infrastructure being widely available, it is time to stop
attempting to build wheelhouses that can install on any series
and architectures.

The perferred method moving forward is to build a binary version
of the charm per series and architecture pair and let the Python
package manager do its job, releiving the charm author from the
manual build dependency resolution chore.

Remove various build dependency workarounds from here as they do
not apply to all charms.

Replace all pins with a less than next major version pin as the
referenced issues have been resolved.

For any charm author that want to continue to use the old approach
they are free to add build dependencies to their wheelhouses.

For older stable branches we recommend people to use the lock
file feature [0][1].

0: juju/charm-tools#585
1: juju/charm-tools#598

Signed-off-by: Frode Nordahl <[email protected]>
Copy link
Contributor

@ajkavanagh ajkavanagh left a comment

Choose a reason for hiding this comment

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

LGTM; I'd like a second opinion too, as it's quite a dramatic change!

@fnordahl fnordahl requested a review from addyess September 20, 2022 11:29
@hloeung
Copy link
Contributor

hloeung commented Oct 19, 2022

What's the minimal series for binary wheels? https://github.com/juju-solutions/layer-basic/pull/205/files seems to suggest Focal?

I'm asking because we still have various places running Bionic and Xenial so some of our charms still need to support those two series/releases.

@ajkavanagh
Copy link
Contributor

What's the minimal series for binary wheels? https://github.com/juju-solutions/layer-basic/pull/205/files seems to suggest Focal?

I'm asking because we still have various places running Bionic and Xenial so some of our charms still need to support those two series/releases.

There's no particularly reason that layer-basic won't support binary charms on xenial & bionic. The big issue is that xenial (py35) and bionic (py36) are basically EOL from a python module perspective. So you should absolutely pin the modules for those versions, and continue to release them as source charms. The only reason that binary charms were introduced is due to the issues with setuptools allowing multiple backends (e.g. poetry-core, flit-core, etc.) and that pip download doesn't to dependency resolution on the backend needed to install the python module; hence building binary wheels bypasses this issue.

As the charmhub is aware of the base for the charm (e.g. 18.04, 20.04), a charm can be released that builds for 18.04 only and then it can be stable. A different branch of the charm can build for 22.04 only and be released to the same channel (e.g. latest) and they can both coexist.

Copy link
Contributor

@ajkavanagh ajkavanagh left a comment

Choose a reason for hiding this comment

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

I've had some more time to mull this over, and I'd really like to suggest that we leave py38 and before pins alone (i.e. they currently work for those charms), and only unpin/re-pin for py310+ (e.g. jammy and later). My reasoning is is that we have a lot of charms that currently work with these pins on layer-basic on py36 and py38 (bionic/focal). Changing layer-basic now would result in significant module changes in charms that were rebuilt for those versions of python.

Obviously, the best solution would be able to version layers and thus not need to use the same layer for multiple python versions. But charm-tools doesn't support that, and with the operator framework, that ship has sailed!

@fnordahl
Copy link
Member Author

fnordahl commented Nov 6, 2022

For the record, the introduction of very specific pinning in this layer is quite recent (commit fb767dc), and as we've come to discuss on other PRs it invites further pollution of this layers wheelhouse.txt that in turn forces bloat in any charms not in need of these packages.

I understand and respect your wish to avoid breaking anyone and keep things working, but at the end of the day the side effect is that it accelerates build-up of tech debt.

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.

4 participants