Skip to content

enhance make#37097

Draft
bircni wants to merge 1 commit intogo-gitea:mainfrom
bircni:feature/enhance-make
Draft

enhance make#37097
bircni wants to merge 1 commit intogo-gitea:mainfrom
bircni:feature/enhance-make

Conversation

@bircni
Copy link
Copy Markdown
Member

@bircni bircni commented Apr 3, 2026

WIP!!!
need feedback from @silverwind as discussed

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 3, 2026
@bircni bircni marked this pull request as draft April 3, 2026 14:23
@bircni bircni requested a review from silverwind April 3, 2026 14:23
.PHONY: watch
watch: ## watch everything and continuously rebuild
@bash tools/watch.sh
watch: ## watch everything (optional: WATCH_DB=sqlite|mysql|pgsql|mssql WATCH_ACT=true)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  1. there is no other "DB" that needs to "watch".
  2. there is no "ACT" at the moment.

Off-topic: maybe it should really introduce this: WIP: Pure Go SQLite3 #32628 , then no build tag is needed anymore.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

afaik you were sort of against it - #20614 (comment)
I wouldn't mind having sqlite built-in without tags though.

Copy link
Copy Markdown
Contributor

@wxiaoguang wxiaoguang Apr 3, 2026

Choose a reason for hiding this comment

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

The wasm one isn't like that CCGO based "pure Go" solution. The wasm one is 100% (at least >99%) complied from official SQLite C code, so I would consider it as safe and stable enough for production.

I was just against CCGO at that time 😄


To explain more for WASM ecosystem: mature compiler, mature VM. CCGO is cool and awesome, wasm is the future (IMHO)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My bad! I assumed this was the same solution (modernrc) and did not look at what it's imported in your PR (ncruces).
seems decent. At least better for us since we don't have to support sqlite tags.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Update: it seems that ncruces changed its solution.

  • Old ncruces: SQLite C -> WASM compiler -> WASM code -> WASM VM
  • New ncruces: SQLite C -> WASM compiler -> WASM code -> ncruces wasm2go -> Go code
  • modernrc: SQLite C -> modernrc CCGO compiler -> Go code

Still, modernrc CCGO is much more complicated than ncruces wasm2go 🤣

Copy link
Copy Markdown
Contributor

@wxiaoguang wxiaoguang Apr 3, 2026

Choose a reason for hiding this comment

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

cznic (native)

we call it "modernc", its package is modernc.org/sqlite

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

modernc.org/sqlite

Right, the module path still is that: https://gitlab.com/cznic/sqlite/-/blob/master/go.mod#L1

Copy link
Copy Markdown
Contributor

@TheFox0x7 TheFox0x7 Apr 3, 2026

Choose a reason for hiding this comment

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

ncruces seems slower to compile from scratch. Not a big issue though. It requires one less workaround but the changes in how parameters are passed seem the same.

Size diff (same base, only changed enough to get it compiling and browsable):
+6MB on ncruces
+1.1MB on modernrc

Respective branches:
https://github.com/TheFox0x7/gitea/tree/modernrc-sqlite
https://github.com/TheFox0x7/gitea/tree/ncurces-sqlite

Let me know if you have something I should try or which one to start as draft

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Some benchmarks at https://github.com/cvilsmeier/go-sqlite-bench, modernc seems slightly faster on average.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

modernc +1

@TheFox0x7
Copy link
Copy Markdown
Contributor

Since you're considering more targets, maybe we could have the "dev instance" one? I still have yet to figure out how can I deploy the exact instance we have preconfigured for tests which makes finding the right setup more complex than it needs to be.

@wxiaoguang
Copy link
Copy Markdown
Contributor

I still have yet to figure out how can I deploy the exact instance we have preconfigured for tests which makes finding the right setup more complex than it needs to be.

Could you elaborate? Didn't get the point.

@TheFox0x7
Copy link
Copy Markdown
Contributor

I worked on some bug here and I had to dive into the db schemas and repositories to find which user has which repository and what's in them to set the test up.
This is stuff which I can get in a minute from a live instance and took a lot longer because I couldn't just browse it.
The closet I got to that was to plant a breakpoint and peek at the running instance during the test but it was - as you might expect - less than ideal. There has to be a sane way to do it and either I'm unaware of it or it is not a thing yet.

I think it's useful for integration tests to see how the system is setup and for example try out your test in UI first to find the good way to perform it. Plus as a general unified demo setup it could work too.

@wxiaoguang
Copy link
Copy Markdown
Contributor

Do you mean something like this ? add develop script to view and update test data #26594 ?

@silverwind
Copy link
Copy Markdown
Member

You mean some sort of database pre-seeding for demo purposes?

@TheFox0x7
Copy link
Copy Markdown
Contributor

Do you mean something like this ? add develop script to view and update test data #26594 ?

Yup. Exactly that. Though it might be out of scope for this change... now that I look at it.

You mean some sort of database pre-seeding for demo purposes?

It's less for a demo more for figuring out a setup for integration test. Demo is just a side effect here.

@wxiaoguang
Copy link
Copy Markdown
Contributor

Do you mean something like this ? add develop script to view and update test data #26594 ?

Yup. Exactly that. Though it might be out of scope for this change... now that I look at it.

Maybe it's better not looking at it. 26594 made things too complicated. For "viewing test data", it can just copy the test fixtures to a temp dir, start a server, that's all. It shouldn't "update" the existing data from the "test view server".

@wxiaoguang
Copy link
Copy Markdown
Contributor

Do you mean something like this ? add develop script to view and update test data #26594 ?

Yup. Exactly that. Though it might be out of scope for this change... now that I look at it.

Maybe it's better not looking at it. 26594 made things too complicated. For "viewing test data", it can just copy the test fixtures to a temp dir, start a server, that's all. It shouldn't "update" the existing data from the "test view server".

By the way, to make the CI/testing system maintainable, we should avoid adding any more test data in to the fixtures.

The tests should create their own test fixtures, or create data dynamically.

@silverwind
Copy link
Copy Markdown
Member

silverwind commented Apr 3, 2026

The tests should create their own test fixtures, or create data dynamically.

Just model after e2e tests. every test creates all dependencies it needs itself. Fully parallelizeable, no shared fixtures, no shared state, can be made arbritrarily fast via more CPU cores. Every test is "pure".

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.

5 participants