Uh oh!
There was an error while loading. Please reload this page.
Support installing charms with built wheels - #205
Conversation
Uh oh!
There was an error while loading. Please reload this page.
| @@ -1,3 +1,3 @@ | |||
| # Use latest so that layer-basic tests against main branch | |||
| git+https://github.com/juju/charm-tools.git#egg=charm-tools | |||
There was a problem hiding this comment.
Need to bring this back once we land juju/charm-tools#620
| if s.endswith(ending): | ||
| return s[:-len(ending)] | ||
| if s.endswith('.whl'): | ||
| return s.split('-', 1)[0].replace('_', '-') |
There was a problem hiding this comment.
I would probably create a small function that gets PEP 503 normalized names:
https://peps.python.org/pep-0503/#normalized-names
import re
def normalize(name):
return re.sub(r"[-_.]+", "-", name).lower()
The binary distribution names should be normalized names with - replaced by _ judging by: https://packaging.python.org/en/latest/specifications/binary-distribution-format/#escaping-and-unicode
I think referring to PEP 503 would make it more clear why we are doing this replacement.
There was a problem hiding this comment.
Looks good to me overall, although I would slightly refactor the
_add_back_versionsfunction modification.
Thanks for the review, Dima. Yes, I'll revisit that one.
To be honest I'm not quite sure why this is needed both in _load_wheelhouse_versions and _add_back_versions as the second gets its input from the former, but I guess it was added for some reason that is not obvious so I'll keep it.
Moving this to a separate helper instead of duplicating the code would be good though.
The PEP 503 reference also makes sense, although I do not particularly like regular expressions when the same can be done with string manipulation.
There was a problem hiding this comment.
Ah, it appears to me that the original authors gave up on fully parsing the variety of source wheel formats out there by using the LooseVersion package, and just including whatever comes after the version string into the version comparison.
Example from the unit test:
{
'python-dateutil': LooseVersion ('2.8.1.tar.gz'),
'setuptools-scm': LooseVersion ('1.17.0.tar.gz'),
'wheel': LooseVersion ('0.33.6.tar.gz'),
}So this is probably why the second method exist.
Appears to me we should probably try to unpickle this a bit, but if we introduce a strict normalization we should probably contain that new handling to the binary wheels, so that we don't mess with existing in the wild setups using the source build method.
There was a problem hiding this comment.
The binary distribution names should be normalized names with - replaced by _ judging by: https://packaging.python.org/en/latest/specifications/binary-distribution-format/#escaping-and-unicode
Regarding this, the existing code is actually relying on the distributed wheels to already follow this normalization, and does the opposite and replaces _ (LOW LINE) and stores the package names with - (HYPHEN) in its internal representation.
I see no need to change this, and the internal handling of both wheel types must be identical in order for upgrades to succeed.
dshcherb
commented
Sep 7, 2022
Looks good to me overall, although I would slightly refactor the |
The file naming convention for Python source wheels and built binary wheels are quite different. Update the code so that it can extract package name and versions from both wheel types. Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com>
fnordahl
commented
Sep 8, 2022
After reviewing the need for the change in the |
Signed-off-by: Frode Nordahl <frode.nordahl@canonical.com>
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 <frode.nordahl@canonical.com>
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 <frode.nordahl@canonical.com>
The file naming convention for Python source wheels and built binary wheels are quite different.
Update the code so that it can extract package name and versions from both wheel types.
Signed-off-by: Frode Nordahl frode.nordahl@canonical.com