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.

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

Remove public/css from git #6782

Closed
silverwind opened this issue Apr 27, 2019 · 17 comments · Fixed by #9114
Closed

Remove public/css from git #6782

silverwind opened this issue Apr 27, 2019 · 17 comments · Fixed by #9114
Labels
type/proposal The new feature has not been accepted yet but needs to be discussed first.

Comments

@silverwind
Copy link
Member

Having public/css tracked in git causes merge conflicts on any CSS change. Can we remove it from git and require a working Node.js installation to build the CSS?

Regarding release builds, can someone shed some light on how they are done? Would we need add Node.js installation to the Dockerfile for the releases to be able to build CSS?

@lafriks
Copy link
Member

lafriks commented Apr 27, 2019

Problem is that you won't be able to build gitea from source without nodejs

@silverwind
Copy link
Member Author

Yes, it would be a build requirement, just like golang is. I really think that is not too much to ask for, it's available pretty much anywhere.

@silverwind
Copy link
Member Author

Also, it will be a requirement for proper frontend JS packaging, so I see it unavoidable to depend on it.

@lunny lunny added the type/proposal The new feature has not been accepted yet but needs to be discussed first. label Apr 28, 2019
@techknowlogick
Copy link
Member

Related to #6203 (this is CSS, the other is about JS)

@silverwind
Copy link
Member Author

It will also require a working internet connection towards npmjs.com to download the modules required for the CSS build.

Generally I'm in favor of downloading all build dependencies (including go modules) on-demand rather then vendoring them.

I see that the golang community prefers to vendor dependencies, which may come from the fact that golang has no central repository for its modules, but in the JS world it's the accepted way to depend on the npm registry. The only other option is to vendor node_modules which was heavily rejected the last time I tried that.

@sapk
Copy link
Member

sapk commented May 2, 2019

@silverwind I think the vendoring for go projects will drop with module (select the needed version from local cache and not the one in GOPATH), the creation of go proxy/cache for Go1.13 (that will limit the downtime and disapearing module) and mostly the fact that depending libs are more stable than before (less removal or rename)

@sapk
Copy link
Member

sapk commented May 2, 2019

For the docker build, we will need them at the build stage to build the binary but not at the last (release) stage.

@silverwind
Copy link
Member Author

silverwind commented May 2, 2019

I think the vendoring for go projects will drop with module

But even with a local cache of go modules, isn't it still quite risky to depend on GitHub repos? When they are deleted, it would break our build, right?

In contrast, npm guarantees that no package is ever deleted (except in a very limited timeframe after publish).

@sapk
Copy link
Member

sapk commented May 2, 2019

Go1.13 will support mirror/proxy that will mitigate that problem (if the licence permit it). https://blog.golang.org/modules2019
https://twitter.com/FiloSottile/status/1123325054794657792?s=19

@lunny
Copy link
Member

lunny commented May 5, 2019

@sapk that will not work on China and as I know also there are some offline workspace which will never not connect to internet.

@sapk
Copy link
Member

sapk commented May 5, 2019

@lunny It already exist some open-source implementation of the proxy. They hope that some proxy will be maintain by local community and the proxy and sum server is configurable.

@silverwind
Copy link
Member Author

@lunny Is a offline build a requirement?

We should decide either to ship everything (vendoring both golang and node modules) or nothing (pull from npm/go proxy on build) in git.

@techknowlogick
Copy link
Member

We can compile and run gitea offline without vendoring node modules, but we can't without vendoring go modules.

@silverwind
Copy link
Member Author

Currently yes, but only because JS modules are vendored in a non-standard fashion. Once those move to node_modules it would need to be vendored to support offline builds. It is standard nowadays to source JS from node_modules, ideally via a bundler like webpack.

@lunny
Copy link
Member

lunny commented May 7, 2019

except nodejs, could we have any other options to organization javascript modules?

@silverwind
Copy link
Member Author

Not that I'm aware of. Bower was at one time the primary frontend package manager, but its days are past and everyone today seems to rely on either npm or yarn, both which build a node_modules folder and bundlers like webpack or parcel source the modules from there.

@silverwind
Copy link
Member Author

silverwind commented Nov 21, 2019

The recent introduction of the JS build process brought up this pain point again for JS. Every time we push a JS change, all pull requests with more JS changes will conflict. This is very bad for testing among other things.

I think it's time to drop them from git and require Node.js as a build-time dependency.

silverwind added a commit to silverwind/gitea that referenced this issue Dec 4, 2019
- Added Node.js as build dependency and removes build files from git.
- Added version checks for both Go and Node.js.
- Overhauled the js/css make target to only run when needed.
- Merged the `generate` make target into `build` as per suggestion.

Fixes: go-gitea#6782
Fixes: go-gitea#9216
lunny pushed a commit that referenced this issue Dec 5, 2019
- Added Node.js as build dependency and removes build files from git.
- Added version checks for both Go and Node.js.
- Overhauled the js/css make target to only run when needed.
- Merged the `generate` make target into `build` as per suggestion.

Fixes: #6782
Fixes: #9216
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type/proposal The new feature has not been accepted yet but needs to be discussed first.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants