Skip to content

Conversation

@silverwind
Copy link
Member

@silverwind silverwind commented Jan 21, 2026

This generates the SVG asset directories before running webpack, eliminating the needs to have the output files checked into git and to run make svg after these dependency updates. The only downside is we can not see changes to the output files during review, but that's acceptable imho.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 21, 2026
@silverwind
Copy link
Member Author

silverwind commented Jan 22, 2026

Test failures indicate the backend may also have a dependency on the SVG assets, so likely we will need to alter make dependencies which will introduce the node dependency of the backend build.

@silverwind silverwind marked this pull request as draft January 22, 2026 01:40
@silverwind
Copy link
Member Author

silverwind commented Jan 22, 2026

will introduce the node dependency of the backend build.

Actually I think it can be avoided. It's just that all tests have the dependency on the svg files and therefor node now, so I've added SVG_DEST to all the various tests targets.

@wxiaoguang
Copy link
Contributor

As an IDE users, before:

  • ./make watch-frontend
  • click the "Start debugger" button in IDE to start backend

After: it needs one more step to build svg before starting the debugger?

@silverwind
Copy link
Member Author

silverwind commented Jan 22, 2026

As an IDE users, before:

* ./make watch-frontend

* click the "Start debugger" button in IDE to start backend

After: it needs one more step to build svg before starting the debugger?

Should work like before, watch-frontend depends on SVG_DEST, so it will build the SVGs before starting webpack (if needed).

@silverwind silverwind marked this pull request as ready for review January 22, 2026 04:02
@silverwind
Copy link
Member Author

After: it needs one more step to build svg before starting the debugger?

To further clarify: Backend does not have a dependency on pre-built SVGs, only tests have this dependency. In development they will be read from the file system and for the prod build it's guaranteed that the public assets are built before the backend is.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jan 22, 2026

Backend does not have a dependency on pre-built SVGs,

Backend does need the pre-built SVGs, backend directly loads SVGs from public/assets/img/svg

Since this: Direct SVG rendering (#12157), https://github.com/go-gitea/gitea/pull/12157/files#diff-315287eed590655a2b96b78db333e10c824282a6764d25f5e13642a8c4386e3c

@silverwind
Copy link
Member Author

silverwind commented Jan 22, 2026

Backend does need the pre-built SVGs, backend directly loads SVGs from public/assets/img/svg

Yes but it's not a requirement for them to exist before the backend dev server starts, it's fine if they are generated with a delay as part of watch-frontend.

@wxiaoguang
Copy link
Contributor

Backend does need the pre-built SVGs, backend directly loads SVGs from public/assets/img/svg

Yes but it's not a requirement for them to exist before the backend dev server starts, it's fine if they are generated with a delay as part of watch-frontend.

It is a requirement for them to exist before the backend dev server stars , see mustInit(svg.Init)

@silverwind
Copy link
Member Author

silverwind commented Jan 22, 2026

Hmm that complicates it. What exactly does click the "Start debugger" button in IDE to start backend do? Which command is ran? Does it run air? Maybe we need to commit some vscode config so that we can pass additional commands.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jan 22, 2026

Hmm that complicates it. What exactly does click the "Start debugger" button in IDE to start backend do? Which command is ran? Does it run air?

Run the entry:

Details image

BTW: I don't mean that it is a must that the assets must be ready before the server's first start. It can tell developers that the assets are missing and restart the server later. I only mean that backend server does need the assets to work.

@silverwind
Copy link
Member Author

silverwind commented Jan 22, 2026

I'd say running main like that is not the recommended way to run the dev server which is still make watch. Maybe we should make make watch also start the debugger (I assume it's dlv?)

Aside that, vscode seems to allow some customization for the debugger startup. Maybe via a preloadTask, https://code.visualstudio.com/docs/debugtest/debugging-configuration#_launchjson-attributes.

But in any case, we need to fix make watch so it builds the SVGs first before starting backend build.

@silverwind silverwind marked this pull request as draft January 22, 2026 04:39
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jan 22, 2026

But in anycase, we need to fix make watch so it builds the SVGs first before starting backend build.

Yep, make watch is another problem.

SVG assets are different from JS/CSS assets. Backend can tolerate that JS/CSS assets are missing and load them later flawlessly (one flaw: theme list will be empty and use default theme). But if SVG assets are missing, the server need to restart to do reload.


Or, need to make backend code also tolerate SVG assets missing and support dynamic reloading

@wxiaoguang
Copy link
Contributor

Maybe we should make make watch also start the debugger (I assume it's dlv?)

Goland debugger:

/.../GoLand.app/Contents/plugins/go-plugin/lib/dlv/macarm/dlv --listen=127.0.0.1:51166 --headless=true --api-version=2 --check-go-version=false --only-same-user=false exec /Users/user/work/gitea/gitea_web -- web #gosetup
API server listening at: 127.0.0.1:51166
...

@silverwind
Copy link
Member Author

silverwind commented Jan 22, 2026

make backend code also tolerate SVG assets missing and support dynamic reloading

I guess we could remove this svgIcons "cache" variable and dynamically read from fs/embed.

var svgIcons map[string]string

@silverwind silverwind requested a review from Copilot January 22, 2026 10:44
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR automates SVG asset generation by integrating it into the build process. Instead of requiring manual make svg commands and checking generated files into git, SVG assets are now generated automatically as build dependencies.

Changes:

  • Modified Makefile to generate SVG assets as a prerequisite for webpack, tests, and linting tasks
  • Updated GitHub Actions workflows to install Node.js and pnpm for backend test jobs
  • Removed the separate svg-check target that verified generated assets were committed

Reviewed changes

Copilot reviewed 2 out of 436 changed files in this pull request and generated 2 comments.

File Description
Makefile Added $(SVG_DEST) as a dependency to webpack, test targets, and linting tasks; removed svg-check target and converted svg to use make dependencies
.github/workflows/pull-db-tests.yml Added pnpm and Node.js setup steps to all database test jobs to support SVG generation during builds

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/internal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants