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.

Sign up for GitHub

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

fix: replace colons with hyphens in charm names #2060

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: replace colons with hyphens in charm names #2060

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1e6defe
fix: replace colons with hyphens in charm names
mr-cal Dec 20, 2024
bccc99e
chore: drop unused code for charm filenames
mr-cal Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions charmcraft/services/package.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,19 +122,23 @@ def pack_charm(
def get_charm_path(self, dest_dir: pathlib.Path) -> pathlib.Path:
"""Get a charm file name for the appropriate set of run-on bases."""
if self._platform:
return dest_dir / f"{self._project.name}_{self._platform}.charm"
platform = self._platform.replace(":", "-")
return dest_dir / f"{self._project.name}_{platform}.charm"
build_plan = models.CharmcraftBuildPlanner.model_validate(
self._project.marshal()
).get_build_plan()
platform = utils.get_os_platform()
build_on_base = bases.BaseName(name=platform.system, version=platform.release)
host_platform = utils.get_os_platform()
build_on_base = bases.BaseName(
name=host_platform.system, version=host_platform.release
)
host_arch = util.get_host_architecture()
for build_info in build_plan:
print(build_info)
if build_info.build_on != host_arch:
continue
if build_info.base == build_on_base:
return dest_dir / f"{self._project.name}_{build_info.platform}.charm"
platform = build_info.platform.replace(":", "-")
return dest_dir / f"{self._project.name}_{platform}.charm"

raise errors.CraftError(
"Current machine is not a valid build platform for this charm.",
Copy link
Contributor Author

@mr-cal mr-cal Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Unless I am mistaken, we could delete everything after line 125 in this function.

I don't think there is a scenario where self._platform doesn't exist.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This may well be true now. It wasn't true when I first implemented this service, but at that point we could have self._platform == None. I'd have to do a deep dive into craft-application to see if that's still valid in the PackageService.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I was suspecting this was needed during the craft-application migration.

I poked around at the service factory and how charmcraft inits the pack service and dropping this appears safe.

Expand Down
4 changes: 2 additions & 2 deletions tests/spread/smoketests/multi-base/basic/expected-charms.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
[email protected]:amd64.charm
[email protected]:amd64.charm
[email protected]-amd64.charm
[email protected]-amd64.charm
test-charm_noble-amd64.charm
2 changes: 1 addition & 1 deletion tests/spread/smoketests/multi-base/one-platform/expected-charms.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
[email protected]:amd64.charm
[email protected]-amd64.charm
31 changes: 27 additions & 4 deletions tests/unit/services/test_package.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,16 +86,39 @@ def test_get_metadata(package_service, simple_charm: BasesCharm, metadata):


@pytest.mark.parametrize(
("bases", "expected_name"),
("build_plan", "expected_name"),
[
(
[SIMPLE_BUILD_BASE],
pytest.param(
[
BuildInfo(
platform="distro-1-test64",
build_on="riscv64",
build_for="riscv64",
base=BaseName("ubuntu", "24.04"),
)
],
"charmy-mccharmface_distro-1-test64.charm",
id="simple",
),
pytest.param(
[
BuildInfo(
platform="[email protected]:riscv64",
build_on="riscv64",
build_for="riscv64",
base=BaseName("ubuntu", "24.04"),
)
],
"[email protected]",
id="multi-base",
),
],
)
def test_get_charm_path(fake_path, package_service, bases, expected_name):
def test_get_charm_path(fake_path, package_service, build_plan, expected_name):
fake_prime_dir = fake_path / "prime"
package_service._build_plan = build_plan
package_service._platform = build_plan[0].platform

charm_path = package_service.get_charm_path(fake_prime_dir)

assert charm_path == fake_prime_dir / expected_name
Expand Down
Loading