Skip to content

Conversation

techknowlogick
Copy link
Member

opening this for discussion.

I use git worktrees to work on various PRs for Gitea, leading me to have sometimes up to 20different copies of node_modules installed. This takes a toll on the available space on my hard drive. pnpm uses hardlinks&symlinks to alleviate this issue.

I understand this takes us a step away from core nodejs tools, but I think for anyone who works on several branches at a time it would benefit them.

I also had to fork the license checker to support analyzing pnpm projects.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 13, 2025
Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

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

pnpm is awesome - but addoption is not to hight so support in other tools is till kinda lacking

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Aug 14, 2025
@6543
Copy link
Member

6543 commented Aug 14, 2025

-> plz ask more devs for lgtm ... as they also have to work with it :)

@6543
Copy link
Member

6543 commented Aug 14, 2025

@TheFox0x7
Copy link
Contributor

My drive welcomes this as well :)

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Aug 14, 2025
@techknowlogick
Copy link
Member Author

-> plz ask more devs for lgtm ... as they also have to work with it :)

@6543 yup, that's why I also requested a review from @silverwind as they have a lot of interaction with the js side of things so I don't want to negatively impact them just for a slight improvement for me.

I requested a review from you as I know you use it with woodpecker already.

Copy link
Member

@delvh delvh left a comment

Choose a reason for hiding this comment

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

I'm neither for nor against it.
I lack the experience with it to judge if pnpm is a good choice or not.

Nevertheless, there seems be a clear opinion from the TOC in favor of it.
Still, it's probably best to wait for approval from the people that are most impacted by it (@silverwind and @wxiaoguang).

@6543 6543 requested a review from wxiaoguang August 14, 2025 20:43
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

Haven't tested it. I am neutral for it.

@silverwind
Copy link
Member

silverwind commented Aug 18, 2025

This adds a new build dependency, so the build docs need to be updated to mention this requirement:

https://github.com/go-gitea/gitea#building

As for the change, I'm -0 because I see no need to change this package manager. gitea's node_modules consumes 629MB currently. Hard linking npm dependencies creates problems when you want to edit a dependency for debugging and has problems across file system boundaries like docker volumes. I see no benefit.

@silverwind
Copy link
Member

silverwind commented Aug 18, 2025

I also had to fork the license checker to support analyzing pnpm projects.

I'd rather we drop the license.txt generation completely, it's been causing too much issues.

@silverwind
Copy link
Member

I guess you can merge this now and we remove the license.txt generation later (along with your fork), if we get consensus on that removal.

Makefile Outdated
@@ -918,7 +922,7 @@ generate-gitignore: ## update gitignore files

.PHONY: generate-images
generate-images: | node_modules
npm install --no-save fabric@6 imagemin-zopfli@7
pnpm install --no-save fabric@6 imagemin-zopfli@7
Copy link
Member

@silverwind silverwind Sep 3, 2025

Choose a reason for hiding this comment

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

pnpm install does not have a --no-save option: https://pnpm.io/cli/install and it therefor errors:

 ERR_PNPM_OPTION_NOT_SUPPORTED  The "add" command currently does not support the no-save option

Copy link
Member

Choose a reason for hiding this comment

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

Apparently there is open issue pnpm/pnpm#1237 about it. I'm not sure how to solve.

Copy link
Member

Choose a reason for hiding this comment

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

I guess what we should do is make a second package.json and lockfile inside the tools directory that holds these dependencies. That way we avoid any pnpm install failures related to those native modules on the main package.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. I separated it out, and added cairo and pixman to the flake so that at least those who use nix can ensure they have the expected build deps. I'll have to add a mention of those two to the docs as turns out that prebuilt versions of canvas aren't available for at least my combination of devices and node versions.

Copy link
Member

@silverwind silverwind Sep 4, 2025

Choose a reason for hiding this comment

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

Looks good. I think there might be some wasm-based svg-to-png solution somewhere which would work everywhere, but this is good enough for now.

Edit: Likely https://github.com/ssssota/svg2png-wasm

@techknowlogick techknowlogick enabled auto-merge (squash) September 3, 2025 23:49
@techknowlogick techknowlogick added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 4, 2025
@techknowlogick techknowlogick merged commit 361e59f into go-gitea:main Sep 4, 2025
26 checks passed
@GiteaBot GiteaBot added this to the 1.26.0 milestone Sep 4, 2025
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 4, 2025
zjjhot added a commit to zjjhot/gitea that referenced this pull request Sep 4, 2025
* giteaofficial/main:
  Switch to pnpm (go-gitea#35274)
  Switch `bitnami` images to `bitnamilegacy` on CI (go-gitea#35402)
  Upgrade dependencies (go-gitea#35384)
  [skip ci] Updated translations via Crowdin

# Conflicts:
#	package-lock.json
techknowlogick pushed a commit that referenced this pull request Sep 5, 2025
add `/.pnpm-store` to .gitignore to not drive git diff crazy with 10k+
changes after .pnpm-store update, introduced by #35274
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/dependencies modifies/docs modifies/frontend modifies/internal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants