Skip to content

chore(legacy-cli): remove unused help and version commands#5777

Open
lengau wants to merge 6 commits intomainfrom
work/merge-remove-legacy
Open

chore(legacy-cli): remove unused help and version commands#5777
lengau wants to merge 6 commits intomainfrom
work/merge-remove-legacy

Conversation

@lengau
Copy link
Contributor

@lengau lengau commented Sep 16, 2025

This is a merge of #5731 and #5773, which both implement this change.

Authors: @Serena6688 and @Nalin-Kumar-Gupta

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run make lint?
  • Have you successfully run make test?

Serena6688 and others added 4 commits September 14, 2025 14:22
… test

This commit removes the legacy CLI modules that are no longer used:
- snapcraft_legacy/cli/help.py
- snapcraft_legacy/cli/version.py
- tests/unit/cli/test_version.py

Also updates references in _runner.py and plugin loader accordingly.
Part of issue #5682.
@lengau lengau force-pushed the work/merge-remove-legacy branch from 2922650 to 5509b9f Compare September 16, 2025 16:50
@lengau lengau requested a review from steinbro September 16, 2025 18:45
@Nalin-Kumar-Gupta
Copy link
Contributor

I have raised a PR on top of this to solve the failing integration tests - #5778

@bepri
Copy link
Member

bepri commented Sep 17, 2025

@Nalin-Kumar-Gupta the only test failures in this PR are an unrelated bug (canonical/craft-providers#829) and a timeout from a test taking too long, though it was working. What failure are you referring to with your new PR?

@Nalin-Kumar-Gupta
Copy link
Contributor

Nalin-Kumar-Gupta commented Sep 17, 2025

@Nalin-Kumar-Gupta the only test failures in this PR are an unrelated bug (canonical/craft-providers#829) and a timeout from a test taking too long, though it was working. What failure are you referring to with your new PR?

Thanks for clarifying! To be more precise, my PR addresses the failure that occurs specifically when running snapcraft --version after the legacy help/version commands were removed. Without restoring @click.version_option, snapcraft --version exits with an error instead of printing the version. The other integration test failures you mentioned (craft-providers bug + timeout) are indeed unrelated. My changes don’t affect those.

tests/spread/core24-suites/manifest/manifest-creation/task.yaml (line 28):

["snapcraft_version"]=$(snapcraft --version|cut -d' ' -f2)

@bepri
Copy link
Member

bepri commented Sep 18, 2025

I'm not able to reproduce that error. I cloned this PR's branch, packed & installed Snapcraft, and it appears to work:

❯ sudo snap install --dangerous snapcraft_8.12.0.post32_amd64.snap
Please tap your YubiKey.
snapcraft 8.12.0.post32 installed

❯ /snap/bin/snapcraft --version
snapcraft 8.12.0.post32

❯ /snap/bin/snapcraft version
snapcraft 8.12.0.post32

@Nalin-Kumar-Gupta
Copy link
Contributor

I'm not able to reproduce that error. I cloned this PR's branch, packed & installed Snapcraft, and it appears to work:

❯ sudo snap install --dangerous snapcraft_8.12.0.post32_amd64.snap
Please tap your YubiKey.
snapcraft 8.12.0.post32 installed

❯ /snap/bin/snapcraft --version
snapcraft 8.12.0.post32

❯ /snap/bin/snapcraft version
snapcraft 8.12.0.post32

Thanks for checking! I just re-tested and both snapcraft --version and snapcraft version work fine after packing & installing. I must have misunderstood earlier — the failures I mentioned weren’t related to this change. You can merge this I have closed my PR.

@lengau
Copy link
Contributor Author

lengau commented Sep 18, 2025

@Nalin-Kumar-Gupta it's true that the legacy entrypoint won't have a --version anymore. So if you access the legacy entrypoint, --version will fail, but I haven't managed to come up with any way to access the legacy entrypoint before the new one intercepts --version.

I tried in a directory with a core20 snap, which would be the primary way of entering a legacy entrypoint. If you or @bepri can come up with a way that this snap (which is published for amd64 on the latest/edge/pr-5777 channel) could get there, then we should go with your PR instead (and make a test case for it).

@Nalin-Kumar-Gupta
Copy link
Contributor

@Nalin-Kumar-Gupta it's true that the legacy entrypoint won't have a --version anymore. So if you access the legacy entrypoint, --version will fail, but I haven't managed to come up with any way to access the legacy entrypoint before the new one intercepts --version.

I tried in a directory with a core20 snap, which would be the primary way of entering a legacy entrypoint. If you or @bepri can come up with a way that this snap (which is published for amd64 on the latest/edge/pr-5777 channel) could get there, then we should go with your PR instead (and make a test case for it).

Okay let me see if I can write a test for this. Reopened my PR.

@Nalin-Kumar-Gupta
Copy link
Contributor

Nalin-Kumar-Gupta commented Sep 28, 2025

@Nalin-Kumar-Gupta it's true that the legacy entrypoint won't have a --version anymore. So if you access the legacy entrypoint, --version will fail, but I haven't managed to come up with any way to access the legacy entrypoint before the new one intercepts --version.

I tried in a directory with a core20 snap, which would be the primary way of entering a legacy entrypoint. If you or @bepri can come up with a way that this snap (which is published for amd64 on the latest/edge/pr-5777 channel) could get there, then we should go with your PR instead (and make a test case for it).

Hi @lengau
The pyproject.toml defines two script entry points:

  • snapcraft = "snapcraft.application:main" (main entry point)
  • snapcraft_legacy = "snapcraft_legacy.cli.__main__:run" (legacy entry point)

While the main snapcraft command intercepts --version before checking for classic fallback, someone could theoretically call snapcraft_legacy --version directly, which would fail if we remove the version functionality from legacy code. I've pushed a test case in the latest commit that demonstrates this edge case here (#5778).

@bepri
Copy link
Member

bepri commented Sep 29, 2025

Interesting. I wonder if anybody uses that? The modern snapcraft entrypoint should fall back to the legacy codebase for core20 snap builds anyways, so I don't see why anybody would want to invoke it that way.

Still, I think that probably means we should continue to support it until Snapcraft 9. Thoughts @lengau?

@Nalin-Kumar-Gupta
Copy link
Contributor

Interesting. I wonder if anybody uses that? The modern snapcraft entrypoint should fall back to the legacy codebase for core20 snap builds anyways, so I don't see why anybody would want to invoke it that way.

Still, I think that probably means we should continue to support it until Snapcraft 9. Thoughts @lengau?

Hi @lengau please review

@lengau
Copy link
Contributor Author

lengau commented Oct 9, 2025

Pretty sure the snapcraft_legacy entrypoint only existed for testing purposes when we migrated that codebase. It's not exposed in the snap (which is the only official way we publish Snapcraft now).

@bepri
Copy link
Member

bepri commented Oct 10, 2025

I see. I was able to invoke it earlier but I must've been mistakenly in the Python virtual environment for Snapcraft. From a normal install, indeed there is no way to invoke it. We should probably remove that entrypoint.

Thanks for the investigation @Nalin-Kumar-Gupta!

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