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

Jump to bottom

refactor: migrate to starflow #5225

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

refactor: migrate to starflow #5225

wants to merge 29 commits into from

Conversation

bepri
Copy link
Contributor

@bepri bepri commented Jan 29, 2025 •
edited
Loading

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run tox run -m lint?
  • Have you successfully run tox run -e test-py310? (supported versions: py39, py310, py311, py312)

Should be reviewable on a commit-by-commit basis. Comments will be left on individual files explaining why some changes were necessary. As some words of encouragement: about 250 of the file changes were purely from make format!

@bepri bepri self-assigned this Jan 29, 2025
bepri
bepri commented Jan 29, 2025
View reviewed changes
.shellcheckrc Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was meant to be temporary, was almost accidentally forever

bepri
bepri commented Jan 29, 2025
View reviewed changes
TESTING.md Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Superceded by changes to HACKING.md

bepri
bepri commented Jan 29, 2025
View reviewed changes
manual-tests.md Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was more or less all outdated information and references to files that were no longer there

bepri
bepri commented Jan 29, 2025
View reviewed changes
runtests.sh Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Outdated - referenced files that were no longer there

bepri
bepri commented Jan 29, 2025
View reviewed changes
tools/environment-setup-local.sh Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file (along with many of the other tools) were either outdated or superceded by the starbase makefile

bepri
bepri commented Jan 29, 2025
View reviewed changes
units.py Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file was outdated, from a time before pytest

@bepri bepri force-pushed the work/starflow-migration branch 3 times, most recently from 43fe672 to 0b7568b Compare January 30, 2025 14:55
lengau
lengau reviewed Jan 30, 2025
View reviewed changes
.github/workflows/qa.yaml
uses: canonical/starflow/.github/workflows/test-python.yaml@main
with:
# Snapcraft currently only tests on Python 3.12 on Ubuntu 24.04
fast-test-platforms: '["ubuntu-24.04"]'
Copy link
Contributor

Choose a reason for hiding this comment

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

@sergiusens should we be using self-hosted runners for this or GH-hosted ones?

lengau
lengau reviewed Jan 30, 2025
View reviewed changes
appveyor.yml Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we even need this file anymore?

lengau
lengau reviewed Jan 30, 2025
View reviewed changes
requirements-noble.txt Outdated Show resolved Hide resolved
lengau
lengau reviewed Jan 30, 2025
View reviewed changes
pyproject.toml Outdated Show resolved Hide resolved
lengau
lengau reviewed Jan 30, 2025
View reviewed changes
pyproject.toml Show resolved Hide resolved
@bepri bepri force-pushed the work/starflow-migration branch from e6cf350 to 14aa893 Compare January 30, 2025 21:08
@bepri bepri force-pushed the work/starflow-migration branch 2 times, most recently from f94b42f to 4f48911 Compare January 30, 2025 22:24
@bepri bepri force-pushed the work/starflow-migration branch from 8b77814 to c87752f Compare January 30, 2025 22:57
bepri added 19 commits January 30, 2025 17:58
@bepri
chore: organize gitignore
a2f3bda 
This commit doesn't remove any entries, it only organizes them into nice categories
@bepri
style: bring in starbase formatter rules
86b36ff
@bepri
test: mark slow tests
8abde29
@bepri
chore: remove outdated/unused top-level files
02a35a5
@bepri
docs: delete now-obsolete TESTING.md
a99ccc0 
The only part of TESTING.md that remained relevant to modern Snapcraft was the information on Spread, which itself was slightly out of date. This migrates that Spread info to HACKING.MD and deletes the rest.
@bepri
docs: update HACKING.md with new environment information
f81f681
@bepri
build: add python-apt wheels
2d8a4c1
@bepri
ci: update workflow files for starflow/uv
dd8aee6
@bepri
style: add pre-commit config
d79164e
@bepri
ci(docs): update readthedocs config to use uv
1a4c27e
@bepri
chore: correct all typing errors
e7f4864
@bepri
style: run prettier
ab86c40
@bepri
ci: configure starflow test run
b687ad4
@bepri
chore: fix shellcheck warning on kde neon extension
9a6c73e
@bepri
chore: move docutils into docs dependency group
b45182f
@bepri
chore: pin craft-application for test failures
7b22826
@bepri
fix(docs): properly ignore lines being too long for now
7a25bf8
@bepri
test(spread): remove nonexistent files from spread.yaml
a5e9cd8
@bepri
ci: make trivy ignore things we don't need to scan
4df5913 
requirements-noble.txt cannot be built by Trivy and there's
no reason for us to scan spread tests
@bepri bepri force-pushed the work/starflow-migration branch from c87752f to 4df5913 Compare January 30, 2025 22:59
@bepri bepri marked this pull request as ready for review January 30, 2025 23:11
@mr-cal
Copy link
Collaborator

mr-cal commented Jan 31, 2025

The spread test failures look like inconsequential changes to yaml formatting, so those should be quick fixes.

mr-cal
mr-cal reviewed Jan 31, 2025
View reviewed changes
Copy link
Collaborator

@mr-cal mr-cal left a comment

Choose a reason for hiding this comment

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

This is spooky (in a good way). I ran make setup and make test-fast and within a few seconds I was running unit tests.

I have a few nitpicks but overall everything looks great! This will significantly improve developing and lower the barrier to entry for community contributions.

I'm wondering if this PR is a good time to merge starbase's git history into Snapcraft. The advantage of doing this is that subsequent starbase commits can be brought into Snapcraft with small and easy git merges.

extensions/desktop/kde-neon-6/launcher-specific
@@ -35,7 +35,7 @@ append_dir QML2_IMPORT_PATH "$SNAP_DESKTOP_RUNTIME/lib/$ARCH"
# Fix locating the QtWebEngineProcess executable
export QTWEBENGINEPROCESS_PATH="$SNAP_DESKTOP_RUNTIME/usr/lib/qt6/libexec/QtWebEngineProcess"
# And QtWebEngine's path to resources
QTWEBENGINE_RESOURCES_PATH="$SNAP_DESKTOP_RUNTIME/usr/share/qt6/resources"
export QTWEBENGINE_RESOURCES_PATH="$SNAP_DESKTOP_RUNTIME/usr/share/qt6/resources"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did shellcheck pick this up?

It seems safe and the correct thing to do. I just need to add this to the next release notes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, Shellcheck found this and I'm pretty sure this change fixes a bunch of Qt related snap issues

common.mk
@echo ::endgroup::
endif

.PHONY: lint-docs
Copy link
Collaborator

Choose a reason for hiding this comment

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

I get this for every make target:

> make lint
Makefile:26: warning: overriding recipe for target 'lint-docs'
common.mk:166: warning: ignoring old recipe for target 'lint-docs'

I'm guessing because it's defined in both files?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. Unfortunately IDK how to override this lint-docs without getting that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm fine to leave this as-is in favor of landing the PR sooner.

common.mk
APT := apt-get
endif

PRETTIER=npm exec --package=prettier -- prettier
Copy link
Collaborator

Choose a reason for hiding this comment

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

pedantic: this line causes make lint to have a few hundred lines of output, one for each file.

Maybe --log-level warn?

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense to me. We can also override this in Makefile if we want.

requirements-noble.txt
@@ -0,0 +1,3 @@
# Update this by finding the latest version of the tarball at https://launchpad.net/ubuntu/noble/+source/python-apt
Copy link
Collaborator

Choose a reason for hiding this comment

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

🙏

schema/snapcraft.json
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this new content or the same content but formatted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The exact same, just a super bad diff. Looks like it just got mass-de-dented.

snap/snapcraft.yaml

version="$(python3 setup.py --version)"
${SNAP}/libexec/snapcraft/craftctl set version="$version"
version=$(PYTHONPATH=/root/parts/snapcraft/install/lib/python3.12/site-packages python3 -c "import snapcraft;print(snapcraft.__version__)")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
version=$(PYTHONPATH=/root/parts/snapcraft/install/lib/python3.12/site-packages python3 -c "import snapcraft;print(snapcraft.__version__)")
version=$(PYTHONPATH=$CRAFT_PART_INSTALL/lib/python3.12/site-packages python3 -c "import snapcraft;print(snapcraft.__version__)")

snapcraft_legacy/internal/mountinfo.py
root_mounts = collections.defaultdict(
list
) # type: Dict[str, List[Mount]] # noqa
root_mounts = collections.defaultdict(list) # type: Dict[str, List[Mount]] # noqa
Copy link
Collaborator

Choose a reason for hiding this comment

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

lol type comments

tests/spread/core24-suites/scriptlets/scriptlet-failures/task.yaml
Comment on lines +3 to +4
restore:
| # If this next line fails, delete it and replace it with the one following it
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you drop this outdated comment and move the pipe symbol back to where it belongs?

This seems like a bug in the formatter.

tests/spread/core24/set-version-twice/task.yaml
@@ -4,7 +4,8 @@ prepare: |
#shellcheck source=tests/spread/tools/snapcraft-yaml.sh
. "$TOOLS_DIR/snapcraft-yaml.sh"

restore: | # If this next line fails, delete it and replace it with the one following it
restore:
| # If this next line fails, delete it and replace it with the one following it
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here

@bepri
Copy link
Contributor Author

bepri commented Feb 3, 2025

I'm wondering if this PR is a good time to merge starbase's git history into Snapcraft. The advantage of doing this is that subsequent starbase commits can be brought into Snapcraft with small and easy git merges.

That's the plan! I'm leaving that rebase out of this PR until it's time to merge so that the commit history is easier on reviewers.

@medubelko medubelko mentioned this pull request Feb 5, 2025
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mr-cal mr-cal mr-cal left review comments

@lengau lengau lengau left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees

@bepri bepri

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bepri @mr-cal @lengau