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

add timetzdata build tag to binary releases #33463

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

techknowlogick
Copy link
Member

timetzdata is already used in the docker images, this includes them for the binary release files too.

Related to #33235 (I don't have a windows machine setup to test this though)

@techknowlogick techknowlogick added topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. backport/v1.23 This PR should be backported to Gitea 1.23 labels Jan 31, 2025
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 31, 2025
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 31, 2025
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jan 31, 2025

Thank you very much for the details. I just found a Go issue: time: add time/tzdata package and timetzdata tag to embed tzdata in program #38017, https://github.com/golang/go/issues/38017

According to the Go issue 38017, it seems that the problem is mainly related to Windows.

Maybe we could add a xxx_windows.go and use import _ "time/tzdata" there? It could save 800KB binary size for non-Windows build, and clearer to our build system (easy to add comments and fine tune and maintain in the future)

@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 Jan 31, 2025
@silverwind
Copy link
Member

Some more context from https://pkg.go.dev/time/tzdata:

If this package is imported anywhere in the program, then if the time package cannot find tzdata files on the system, it will use this embedded information.
Importing this package will increase the size of a program by about 450 KB.

We know some versions of Windows are affected, but it may affect others as well, like for example weird BSD variants etc. I think it's better to always include the fallback tzdata info. 450kB seems acceptable.

@silverwind
Copy link
Member

silverwind commented Feb 1, 2025

Oh, and I think it would be great if we specify such default tags in Makefile instead of the actions, so that people who build from source can also benefit:

gitea/Makefile

Line 136 in 47bf836

TAGS ?=

@wxiaoguang
Copy link
Contributor

We know some versions of Windows are affected, but it may affect others as well, like for example weird BSD variants etc. I think it's better to always include the fallback tzdata info. 450kB seems acceptable.

Why pay the cost which doesn't really affect?

On a slightly related note, I would like to see tags like this being specified as the default tags in Makefile here:

Why not just use xxx_windows.go / xxx_freebsd.go to import _ "time/tzdata"?

@silverwind
Copy link
Member

silverwind commented Feb 1, 2025

We know some versions of Windows are affected, but it may affect others as well, like for example weird BSD variants etc. I think it's better to always include the fallback tzdata info. 450kB seems acceptable.

Why pay the cost which doesn't really affect?

On a slightly related note, I would like to see tags like this being specified as the default tags in Makefile here:

Why not just use xxx_windows.go / xxx_freebsd.go to import _ "time/tzdata"?

Because you can't know for certain whether a given OS has tzdata available. Let's say I compile a barebone Linux distro like Arch or Gentoo and forget to install that package. Gitea would not start up.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Feb 1, 2025

Because you can't know for certain whether a given OS has tzdata available. Let's say I compile a barebone Linux distro like Arch and forget to install that package. Gitea would not start up.

No, it WOULD start up. No timezone data affects nothing as long as the end user doesn't set it in their config.

@silverwind
Copy link
Member

You mean #33235 (comment) does not happen in default config? Well, even if that's true, I think it's still good to include those 450kB just in case. This is only a 0.5% size increase assuming a 100mb binary.

@wxiaoguang
Copy link
Contributor

You mean #33235 (comment) does not happen in default config? Well, even if that's true, I think it's still good to include those 450kB just in case. This is only a 0.5% size increase assuming a 100mb binary.

Your worry will never happen. Only Windows users need that timezone data.

  • For a Linux user, if they set timezone manually but the OS doesn't contain tzdata: then they could realize that they need to install the tzdata package.
  • For a Windows user: there is no way to install tzdata, so the tzdata must be packed into the binary.

@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 4, 2025
@github-actions github-actions bot added modifies/go Pull requests that update Go code and removed modifies/internal labels Feb 4, 2025
@techknowlogick
Copy link
Member Author

updated per @wxiaoguang feedback.

I chose settings module as location for import since we already have other conditional imports there, such as sqlite.

@wxiaoguang
Copy link
Contributor

I will add some comments

@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 Feb 5, 2025
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/v1.23 This PR should be backported to Gitea 1.23 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/go Pull requests that update Go code reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Service Not Running When Running 1.23.1 for Update from 1.22.6 to 1.23.1 in Windows OS
6 participants