ci: refactor main workflow with just and pre-commit #8

florianvazelle · 2023-12-26T16:15:09Z

Hi 👋

I love this addon, it's very great to download assets and prototype very quickly. You introduced me to reuse, which is really what I was looking for to track assets when I start to develop games, and which I now use all the time. So, I want to maintain glam, and migrate it to Godot 4.

But, firstly, I want to propose a better dev & CI workflow, to launch easily the CI locally.

I propose just as command runner, allowing to very cleanly setup useful commands, with env vars support.

$ just
OS: linux - ARCH: x86_64
Available recipes:
    bump-version       # Updates the addon version
    ci-godot-x11 *ARGS # Starts godot using Xvfb and pulseaudio
    ci-load-dotenv     # Add some variables to Github env
    clean              # Remove any unnecessary files
    clean-addons       # Remove plugins
    default            # Display all commands
    editor             # Open the Godot editor
    fmt                # Run files formatters
    godot *ARGS        # Godot binary wrapper
    import-resources   # Import game resources
    install-addons     # Download plugins
    integration        # Run integration tests
    publish            # Upload the addon on Github
    unit               # Run unit tests

I propose pre-commit to run linters and formattes, like codespell, reuse (in the project & the addon directory), and some other hooks which allows you to format several file formats. Like the hooks you made, it can be launched automatically at each commit, or via just fmt.
I remove the dependency with Node.js for integration tests. The project already use python for gdformat, and the http.server of python have same behavior.
And I include a simple dependabot configuration, that bump github actions used in the CI.

Notes:

The CD part (the publish job) is launched when a tag is pushed. The addon version in the .env must be equal to the tag. And the step automatically bump addon and create a github release.
The custom/complex part of the CI is moved to a local action that configures mesa3d, scream, x11 ... because your setup-godot action doesn't seems to work anymore

I'm open to feedback, it's a development workflow I use personally, so I'm familiar with it 🙂

@florianvazelle

Fixes AmbientCG assets not loading. Cherry-picked from the awesome PR #8 by @florianvazelle.

lihop · 2023-12-28T09:59:09Z

Hi @florianvazelle

Thanks for the PR. I'm glad you like the addon and that I could introduce you to reuse. Happy to have you onboard to maintain the project and port it to Godot 4!

It's been a while since I worked on this project, and a lot of these tools are new to me, so I'll need some time to get up to speed and review this PR properly, but I like what I see so far.

Here is some initial feedback:

I propose just as command runner, allowing to very cleanly setup useful commands, with env vars support.

I hadn't heard of Just, so thanks for introducing me. It looks like a good tool that will be useful for the project. I have some shell scripts is some other projects (e.g. build.sh) that would also probably benefit from being converted in to Justfiles.

I propose pre-commit to run linters and formattes, like codespell, reuse

pre-commit looks great too. Thanks for adding codespell. I see it's already found a bunch of typos and spelling mistakes 😅

I remove the dependency with Node.js for integration tests. The project already use python for gdformat, and the http.server of python have same behavior.

Great call 👍

The custom/complex part of the CI is moved to a local action that configures mesa3d, scream, x11 ... because your setup-godot action doesn't seems to work anymore

I really like what you have done with the complex part of the CI, like using the composite action and ssciwr/setup-mesa-dist-win to install mesa-dist-win. it is a lot simpler than how setup-godot is currently written. I do use the setup-godot action for several other projects, so ideally would like to maintain this code in one place. Therefore, I will consider cherry-picking some of these changes over to that repo.

That's all for now. Thanks again for the PR and I hope to get back to you with a proper review soon.

florianvazelle · 2023-12-31T08:57:29Z

Thanks for your initial feedback 🙂

Perhaps I can improve the README "developping" section to reflect what I've done in the MR, it might help to understand the different tools ?

lihop · 2023-12-31T23:01:02Z

Updating the Development section of the README sounds like a good idea. Even if it's just some brief documentation with links to third-party docs.

florianvazelle · 2024-01-03T17:41:53Z

Updated ! 🙂

lihop

Here are some initial comments.

I've already cherry-picked the dependabot stuff and I've also gone through and made the code gdlint compliant in anticipation of the gdlint pre-commit hook.

The setup-godot action is working now, so I think we can remove the stuff related to that.

I will have a closer look at the Justfile soon, but I'm not so keen on it downloading the Godot binaries as I feel it clutters up the file quite a bit. I think just having a GODOT env var (defaulting to godot) which is the path to the Godot binary would simplify things. Then leave it up to the user to manage Godot installation as they see fit.

The GitHub workflow seems quite good, although currently not all the integration tests are running. I've also installed pre-commit.ci in the project to do additional checks.

In any case, I'm really grateful that you introduced me to pre-commit. It's a game changer, and I'm looking forward to using it in my other projects.

lihop · 2024-01-09T09:06:57Z

.codespellrc

+skip = assets/**,test/**,addons/gd-plug/**,LICENSES/**,addons/glam/LICENSES/**,addons/glam/.LICENSES/**
+ignore-words-list = acess


suggestion: Remove 'acess' from the ignored words list and skip CREDITS.md.

This will prevent missing real misspellings of 'access', and CREDITS.md is auto-generated by GLAM, so any misspellings in that file on GLAM's side should be caught by other checks.

Suggested change

skip = assets/**,test/**,addons/gd-plug/**,LICENSES/**,addons/glam/LICENSES/**,addons/glam/.LICENSES/**

ignore-words-list = acess

skip = CREDITS.md,assets/**,test/**,addons/gd-plug/**,LICENSES/**,addons/glam/LICENSES/**,addons/glam/.LICENSES/**

lihop · 2024-01-13T06:46:46Z

test/integration/streaming/http_audio_stream_player.test.gd

@@ -17,7 +17,7 @@ var server_pid: int

 func before_all():
 	var path := ProjectSettings.globalize_path("res://test/integration/streaming")
-	server_pid = OS.execute("npx", ["http-server", "--port=%d" % PORT, path], false)
+	server_pid = OS.execute("python", ["-m", "http.server", "%d" % PORT, "-d", path], false)


blocker: Unfortunately this doesn't work as python's http.server does not support HTTP range requests which are used for some of the tests, so I think we should stick with npx for now.

lihop · 2024-01-13T07:13:29Z

Justfile

+    # TODO: Add an asset-lib publish step
+
+# Run unit tests
+unit: install-addons import-resources


suggestion: Remove install-addons and import-resources recipes.

Running these recipes as dependencies slows down this recipe significantly which isn't ideal when running the unit tests frequently.

with:

just unit
...
real 0m13.815s

without:

just unit
...
real 0m3.360s

lihop · 2024-01-13T07:27:28Z

plug.gd

@@ -4,7 +4,7 @@ extends "res://addons/gd-plug/plug.gd"


 func _plugging():
-	plug("bitwes/Gut", {commit = "70c08aebb318529fc7d3b07f7282b145f7512dee"})
+	plug("bitwes/Gut", {commit = "18f6bddf7010b01754d6feb5f96557214e3ead8c"})


suggestion: This could be changed to make the Gut version clear from a glance.

Suggested change

plug("bitwes/Gut", {commit = "18f6bddf7010b01754d6feb5f96557214e3ead8c"})

plug("bitwes/Gut", {tag = "v4.7.1"})

I'm not sure why the other dependencies in plug.gd use commit rather than tag, but I think it's because I didn't realize at the time that gd-plug supported it.

lihop · 2024-01-13T10:34:10Z

.github/dependabot.yml

praise: Thanks for this. I have cherry-picked the commit that adds this file and it works well.

lihop · 2024-01-13T10:54:10Z

.gitattributes

-/*.license export-ignore
+# Exclude all top-level files and directories (except addons) from zip downloads.
+# This makes installing through the AssetLib easier, because no files and folders
+# need to be unchecked.


praise: This is a great comment. Should definitely keep this.

lihop · 2024-01-13T11:03:59Z

.gitattributes

+# need to be unchecked.
+/**				        export-ignore
+/addons/glam			!export-ignore
+/addons/glam/**			!export-ignore


blocker: This is nice, but unfortunately doesn't work. It results in an empty zip archive. There is currently a check in the workflow for this which fails with this .gitattributes file.

glam/.github/workflows/main.yml

Lines 78 to 115 in 43a9fbe

# Git archive should only include addons/glam directory.

check-archive:

name: 'Check Archive'

runs-on: ubuntu-latest

steps:

- uses: actions/checkout@v4

- name: Create git archive

run: git archive -o archive.zip HEAD

- name: Extract archive

run: mkdir -p /tmp/unzipped && unzip archive.zip -d /tmp/unzipped

- name: Copy extracted archive to working directory

run: cp -avr /tmp/unzipped ./unzipped

- name: REUSE compliance check

uses: fsfe/reuse-action@v2

with:

args: --root ./unzipped/addons/glam lint

- name: Check that archive only contains addons directory

run: |

shopt -s nullglob dotglob

ls -laR /tmp/unzipped

files=(/tmp/unzipped/*)

if [ ${#files[@]} -ne 1 ]; then

echo "Wrong number of files in archive (${#files[@]}) expected 1."

exit 1

fi

if [ ! -d "/tmp/unzipped/addons" ]; then

echo "Expected directory (addons) not found."

exit 1

fi

files=(/tmp/unzipped/addons)

if [ ${#files[@]} -ne 1 ]; then

echo "Wrong number of files in addons directory (${#files[@]}) expected 1."

exit 1

fi

if [ ! -d "/tmp/unzipped/addons/glam" ]; then

echo "Expected directory (addons/glam) not found."

exit 1

fi

Adding files and forgetting to update .gitattributes has caught me out a lot. It would be nice if there were a pre-commit hook for it. I'm considering adding one to setup-godot as I would also use it in other projects.

lihop · 2024-01-13T11:09:48Z

.pre-commit-config.yaml

+  rev: v2.0.0
+  hooks:
+  - id: reuse
+  - id: reuse


typo: reuse appears twice here.

lihop · 2024-01-13T11:19:09Z

Justfile

+
+# Run integration tests
+integration: install-addons import-resources
+    just godot --no-window -s addons/gut/gut_cmdln.gd -gdir=res://test/integration/sources -gexit


issue: This is only running some of the integration tests. To run all, it should be:

Suggested change

just godot --no-window -s addons/gut/gut_cmdln.gd -gdir=res://test/integration/sources -gexit

just godot --no-window -s addons/gut/gut_cmdln.gd -gdir=res://test/integration -gexit

lihop · 2024-01-13T11:23:39Z

.gitignore

@@ -14,3 +14,4 @@ mono_crash.*
 todo.gd
 .gut_editor_config.json
 *.swp
+venv/


tought: With pre-commit sandboxing the python git hooks, I don't think we need any reference to python in this project. Therefore, we could also remove the .venv/ ignore from this file.

florianvazelle added 7 commits December 25, 2023 14:25

ci: add dependabot for github actions

54fa375

chore: exclude all top level files and directories

1f105b7

ci: use just as command runner

46d2c32

style: apply new formatters

317f541

chore: bump Gut

81b107d

chore: fix the clean-addons command to not remove recursively

3ab598e

test: replace npx with python to start the http server

1f0b9fa

lihop added a commit that referenced this pull request Dec 28, 2023

Cast value to int

bf4c72e

Fixes AmbientCG assets not loading. Cherry-picked from the awesome PR #8 by @florianvazelle.

chore: improve the developing readme section

ccbf56d

lihop reviewed Jan 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

ci: refactor main workflow with just and pre-commit #8

ci: refactor main workflow with just and pre-commit #8

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

		skip = assets/,test/,addons/gd-plug/,LICENSES/,addons/glam/LICENSES/,addons/glam/.LICENSES/
		ignore-words-list = acess

	skip = assets/,test/,addons/gd-plug/,LICENSES/,addons/glam/LICENSES/,addons/glam/.LICENSES/
	ignore-words-list = acess
	skip = CREDITS.md,assets/,test/,addons/gd-plug/,LICENSES/,addons/glam/LICENSES/,addons/glam/.LICENSES/

	plug("bitwes/Gut", {commit = "18f6bddf7010b01754d6feb5f96557214e3ead8c"})
	plug("bitwes/Gut", {tag = "v4.7.1"})

	# Git archive should only include addons/glam directory.
	check-archive:
	name: 'Check Archive'
	runs-on: ubuntu-latest
	steps:
	- uses: actions/checkout@v4
	- name: Create git archive
	run: git archive -o archive.zip HEAD
	- name: Extract archive
	run: mkdir -p /tmp/unzipped && unzip archive.zip -d /tmp/unzipped
	- name: Copy extracted archive to working directory
	run: cp -avr /tmp/unzipped ./unzipped
	- name: REUSE compliance check
	uses: fsfe/reuse-action@v2
	with:
	args: --root ./unzipped/addons/glam lint
	- name: Check that archive only contains addons directory
	run: \|
	shopt -s nullglob dotglob
	ls -laR /tmp/unzipped
	files=(/tmp/unzipped/*)
	if [ ${#files[@]} -ne 1 ]; then
	echo "Wrong number of files in archive (${#files[@]}) expected 1."
	exit 1
	fi
	if [ ! -d "/tmp/unzipped/addons" ]; then
	echo "Expected directory (addons) not found."
	exit 1
	fi
	files=(/tmp/unzipped/addons)
	if [ ${#files[@]} -ne 1 ]; then
	echo "Wrong number of files in addons directory (${#files[@]}) expected 1."
	exit 1
	fi
	if [ ! -d "/tmp/unzipped/addons/glam" ]; then
	echo "Expected directory (addons/glam) not found."
	exit 1
	fi

	just godot --no-window -s addons/gut/gut_cmdln.gd -gdir=res://test/integration/sources -gexit
	just godot --no-window -s addons/gut/gut_cmdln.gd -gdir=res://test/integration -gexit

ci: refactor main workflow with just and pre-commit #8

Are you sure you want to change the base?

ci: refactor main workflow with just and pre-commit #8

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!