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

change default pid location from /var/run/gitea.pid to /run/gitea.pid #12391

Closed
wants to merge 3 commits into from

Conversation

flokli
Copy link
Contributor

@flokli flokli commented Jul 31, 2020

The former one is only kept for backwards compat according to
https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch05s13.html, and is a
symlink to /run/ on modern distributions.

gitea already defaults to the location shown in the example, so the
argument doesn't have any effect. Instead point to a custom location to
make it more obvious.
…gitea.pid

The former one is only kept for backwards compat according to
https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch05s13.html, and is a
symlink to `/run/` on modern distributions.
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jul 31, 2020
@zeripath zeripath added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Jul 31, 2020
@zeripath
Copy link
Contributor

unfortunately breaking because it will need to be noted on the release notes even if /var/run is just a symlink.

@zeripath zeripath added the topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile label Jul 31, 2020
@zeripath zeripath added this to the 1.13.0 milestone Jul 31, 2020
@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 Jul 31, 2020
Copy link
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

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

Thanks for PR :) This is indeed a beneficial change.

I am hesitant to accept this change as there are many Gitea users running on non-modern OSes that don't have the /run/ directory. A possible solution (as suggested by @zeripath in chat) would be to compile the option in via linker, similar to how we provide the option for various OSes to support FHS compliance. That way Gitea can maintain /var/run as default, but would allow nix, and other OSes to set the default as needed when packaging.

@@ -41,7 +41,7 @@ and it takes care of all the other things for you`,
},
cli.StringFlag{
Name: "pid, P",
Value: "/var/run/gitea.pid",
Value: "/run/gitea.pid",
Copy link
Contributor

Choose a reason for hiding this comment

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

If we moved this value out in to a var then this could be made compile/link time settable

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Value: "/run/gitea.pid",

@flokli
Copy link
Contributor Author

flokli commented Jul 31, 2020

Can you give an example of such other variables that can be set at link time? I'm not really sure I understand what's proposed.

On the other hand, I'm not sure if it's much of an issue in general - What OSes still don't have /run at all currently, but run latest gitea?
We could add something to the release notes, and worst case gitea would fail to start, until either the packager of that distribution, or the admin (who manually upgrades gitea anyhow) would need to change the command line to pass a custom pidfile argument…

IMHO the gitea upstream default should not be the deprecated /var/run, but /run (if creating a pidfile at all is necessary), and whatever extra configuration is required to support super-old distros only working with very legacy locations handled downstream.

@kolaente
Copy link
Member

kolaente commented Aug 1, 2020

@flokli The version variable for example is set at link time: https://github.com/go-gitea/gitea/blob/master/Makefile#L88

@zeripath
Copy link
Contributor

zeripath commented Aug 1, 2020

@flokli also see: https://docs.gitea.io/en-us/install-from-source/#changing-the-default-custompath-customconf-and-appworkpath

Which is how you can build a FHS compliant Gitea

@techknowlogick
Copy link
Member

per @zeripath's suggestion, if LDFLAGS="-X \"code.gitea.io/gitea/modules/setting.CustomPID=/run/gitea.pid\"" is exported as an environment variable (and the make file is used to build), and the line that @zeripath commented on is removed, then the variable is able to be set during build time for the modern distros.

zeripath added a commit that referenced this pull request Aug 15, 2020
#12391 offered to change the default PID file from /var/run/gitea.pid however in discussion it was decided that this could break users of older systems. An alternative was offered that we could make the PID file compile/link time settable.

This PR does this, and changes the name of the setting from CustomPID to simply PIDFile. It also updates the from-source docs to show how to change the compiler settings to do this.

Closes #12391

Signed-off-by: Andrew Thornton <art27@cantab.net>
Co-authored-by: Florian Klink <flokli@flokli.de>
@zeripath
Copy link
Contributor

@flokli Thanks for the idea to fix this!

@flokli
Copy link
Contributor Author

flokli commented Aug 15, 2020 via email

@zeripath
Copy link
Contributor

Centos 7 is the problem.

@flokli
Copy link
Contributor Author

flokli commented Aug 15, 2020 via email

@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
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! 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.

None yet

5 participants