Use correct path to snap binary #17274

techknowlogick · 2021-10-08T19:07:20Z

techknowlogick commented

2021-10-08 19:07:20 +00:00

replace #17157
Fixes #16209

co-authored-by: @OleHusgaard

replace #17157 Fixes #16209 co-authored-by: @OleHusgaard

techknowlogick reviewed 2021-10-08 21:30:54 +00:00

snap/snapcraft.yaml

techknowlogick commented

2021-10-08 21:30:54 +00:00

This fails, as this reports 2021-10-08T21:14:31Z gitea.web[30871]: 2021/10/08 21:14:31 ...s/install/setting.go:21:PreloadSettings() [I] AppPath: $(echo "/snap/gitea/x3/gitea"|sed "s/x3/current/")

This fails, as this reports `2021-10-08T21:14:31Z gitea.web[30871]: 2021/10/08 21:14:31 ...s/install/setting.go:21:PreloadSettings() [I] AppPath: $(echo "/snap/gitea/x3/gitea"|sed "s/x3/current/")`

zeripath reviewed 2021-10-08 22:00:39 +00:00

modules/setting/setting.go

						
				@ -433,2 +433,3 @@

				func getAppPath() (string, error) {

					var appPath string

					appPath := AppPath

					if giteaAppPath, ok := os.LookupEnv("GITEA_APP_PATH"); ok {

zeripath commented

2021-10-08 21:57:43 +00:00

This means that we now have another linker settable default value so it should be included here

https://github.com/go-gitea/gitea/blob/main/docs/content/doc/installation/from-source.en-us.md#changing-default-paths

This means that we now have another linker settable default value so it should be included here https://github.com/go-gitea/gitea/blob/main/docs/content/doc/installation/from-source.en-us.md#changing-default-paths

modules/setting/setting.go

						
				@ -434,1 +434,3 @@

					var appPath string

					appPath := AppPath

					if giteaAppPath, ok := os.LookupEnv("GITEA_APP_PATH"); ok {

						appPath = giteaAppPath

zeripath commented

2021-10-08 21:58:56 +00:00

This should be documented in https://github.com/go-gitea/gitea/blob/main/docs/content/doc/advanced/environment-variables.en-us.md#gitea-files

modules/setting/setting.go

zeripath commented

2021-10-08 21:59:49 +00:00

This and line 469 should be documented in https://github.com/go-gitea/gitea/blob/main/docs/content/doc/advanced/environment-variables.en-us.md#operating-system-specifics or perhaps a new section.

techknowlogick reviewed 2021-10-09 00:58:43 +00:00

modules/setting/setting.go

						
				@ -433,2 +433,3 @@

				func getAppPath() (string, error) {

					var appPath string

					appPath := AppPath

					if giteaAppPath, ok := os.LookupEnv("GITEA_APP_PATH"); ok {

techknowlogick commented

2021-10-09 00:58:42 +00:00

added

techknowlogick reviewed 2021-10-09 00:58:48 +00:00

modules/setting/setting.go

						
				@ -434,1 +434,3 @@

					var appPath string

					appPath := AppPath

					if giteaAppPath, ok := os.LookupEnv("GITEA_APP_PATH"); ok {

						appPath = giteaAppPath

techknowlogick commented

2021-10-09 00:58:48 +00:00

added

techknowlogick reviewed 2021-10-09 01:02:20 +00:00

modules/setting/setting.go

techknowlogick commented

2021-10-09 01:02:20 +00:00

added

wxiaoguang reviewed 2021-10-09 02:31:37 +00:00

modules/setting/setting.go

wxiaoguang commented

2021-10-09 02:31:37 +00:00

I think we should be more careful about the string replacement. Snap document says that the SNAP_REVISION can be something like "x1" (https://snapcraft.io/docs/environment-variables) , a very short string. Maybe there will be other revision strings in future.

So maybe we should just split the appPath and check if the second last field is SNAP_REVISION and replace it, and check the gitea binary existence again. And print a log here to tell users we changed the app path.

I think we should be more careful about the string replacement. Snap document says that the SNAP_REVISION can be something like "x1" (https://snapcraft.io/docs/environment-variables) , a very short string. Maybe there will be other revision strings in future. So maybe we should just split the appPath and check if the second last field is SNAP_REVISION and replace it, and check the gitea binary existence again. And print a log here to tell users we changed the app path.

wxiaoguang reviewed 2021-10-09 02:34:55 +00:00

docs/content/doc/advanced/environment-variables.en-us.md

wxiaoguang commented

2021-10-09 02:34:55 +00:00

SNAP and SNAP_REVISION are not intended to be used by real users. So Maybe they can be written in other sections and warn users do not set these variables manually.

`SNAP` and `SNAP_REVISION` are not intended to be used by real users. So Maybe they can be written in other sections and warn users do not set these variables manually.

wxiaoguang reviewed 2021-10-09 02:36:03 +00:00

docs/content/doc/advanced/environment-variables.en-us.md

wxiaoguang commented

2021-10-09 02:36:03 +00:00

Can we explain why GITEA_APP_PATH sometimes should be set manually? Otherwise it seems a little confusing.

techknowlogick reviewed 2021-10-09 03:04:28 +00:00

docs/content/doc/advanced/environment-variables.en-us.md

techknowlogick commented

2021-10-09 03:04:28 +00:00

It shouldn't be set usually, but in some rare cases it may need to be set to a symlink (ex. In the case of snap)

wxiaoguang reviewed 2021-10-09 03:08:41 +00:00

docs/content/doc/advanced/environment-variables.en-us.md

wxiaoguang commented

2021-10-09 03:08:41 +00:00

Yep, maybe this point can be described in the document, then there will be no confusing.

zeripath reviewed 2021-10-09 09:19:41 +00:00

docs/content/doc/advanced/environment-variables.en-us.md

zeripath commented

2021-10-09 09:19:41 +00:00

- `GITEA_APP_PATH`: Absolute path of gitea binary. (This is usually automatically detected and should therefore be only set in limited circumstances.)

```suggestion - `GITEA_APP_PATH`: Absolute path of gitea binary. (This is usually automatically detected and should therefore be only set in limited circumstances.) ```

zeripath reviewed 2021-10-09 09:44:19 +00:00

docs/content/doc/advanced/environment-variables.en-us.md

zeripath commented

2021-10-09 09:44:19 +00:00

This SHOULD NOT be normally be set.

We absolutely need to discourage users from setting this but the more information we give about it the more likely that they are to set this - and do so incorrectly. However, if it is present it does need to be somewhere in the list of things that can affect Gitea.

(In some ways if I could think of a way of guaranteeing being able to create a directory that executables could be run from we could prevent the hook problem (and every hook related problem) by being able to set git config core.hooksPath to a Gitea owned path. The other half of the underlying problem relating to the authorized_keys file can be solved using a different template which is already possible.)

This SHOULD NOT be normally be set. We absolutely need to discourage users from setting this but the more information we give about it the more likely that they are to set this - and do so incorrectly. However, if it is present it does need to be somewhere in the list of things that can affect Gitea. (In some ways if I could think of a way of guaranteeing being able to create a directory that executables could be run from we could prevent the hook problem (and every hook related problem) by being able to set `git config core.hooksPath` to a Gitea owned path. The other half of the underlying problem relating to the `authorized_keys` file can be solved using a different template which is already possible.)

OleHusgaard (Migrated from github.com) reviewed 2021-10-09 11:20:47 +00:00

snap/snapcraft.yaml

OleHusgaard (Migrated from github.com) commented

2021-10-09 11:20:46 +00:00

Yes, I tried a lot of different variations of this, and could get nothing to work. Sorry for not getting back to you about this.

OleHusgaard (Migrated from github.com) reviewed 2021-10-09 11:37:22 +00:00

docs/content/doc/advanced/environment-variables.en-us.md

OleHusgaard (Migrated from github.com) commented

2021-10-09 11:37:22 +00:00

This SHOULD NOT be normally be set.

I do not think we need this environment variable to fix the issue at hand. The idea for it was that we could then set it in snapcraft.yaml and avoid snap-specific code in setting.go. But unfortunately it seems impossible to set it correctly in snapcraft.yaml.

> This SHOULD NOT be normally be set. I do not think we need this environment variable to fix the issue at hand. The idea for it was that we could then set it in snapcraft.yaml and avoid snap-specific code in setting.go. But unfortunately it seems impossible to set it correctly in snapcraft.yaml.

OleHusgaard (Migrated from github.com) reviewed 2021-10-09 11:47:53 +00:00

modules/setting/setting.go

OleHusgaard (Migrated from github.com) commented

2021-10-09 11:47:53 +00:00

I agree. Then the code would be like what I proposed in PR 17157, but with an added check that the second last field is SNAP_REVISION. That would also include a check with exec.LookPath() that the modified path will actually work.

zeripath reviewed 2021-10-09 12:15:33 +00:00

docs/content/doc/advanced/environment-variables.en-us.md

zeripath commented

2021-10-09 12:15:33 +00:00

Doesn't snap just plonk a symlink in /usr/bin/gitea? Why can't you set GITEA_APP_PATH=/usr/bin/gitea?

(Or better not rely on environment variables - compile the correct path in instead?)

Doesn't snap just plonk a symlink in `/usr/bin/gitea`? Why can't you set `GITEA_APP_PATH=/usr/bin/gitea`? (Or better not rely on environment variables - compile the correct path in instead?)

wxiaoguang reviewed 2021-10-09 16:34:09 +00:00

docs/content/doc/advanced/environment-variables.en-us.md

wxiaoguang commented

2021-10-09 16:34:09 +00:00

@zeripath It seems that snap doesn't create symlink in /usr/bin by default now, https://askubuntu.com/questions/1314018/snap-app-cant-see-usr-bin , https://stackoverflow.com/questions/65586325/ubuntu-path-tries-to-execute-from-snap-folder-instead-of-usr-bin-terraform , unless snap install --classic is used.

I agree with OleHusgaard, if there is a way to fix the problem by tuning snapcraft.yaml, we had better to use it.

However as today it seems that none of us have confidence to do the fix correctly in snapcraft.yaml, but the problem really exists and it makes snap users panic when they do upgrade.

Now we have 3 easy choices:

Tell users to install Gitea snap with --classic and use /usr/bin/gitea for GITEA_APP_PATH, it makes our snap package not that modern.
Replace snap revision path to current path (as above), I prefer this one.
Drop snap support, recommend users to use Docker instead of snap.

Key problem here is none of us is snap expert, maybe in future we can find a better way.

@zeripath It seems that snap doesn't create symlink in `/usr/bin` by default now, https://askubuntu.com/questions/1314018/snap-app-cant-see-usr-bin , https://stackoverflow.com/questions/65586325/ubuntu-path-tries-to-execute-from-snap-folder-instead-of-usr-bin-terraform , unless `snap install --classic` is used. I agree with OleHusgaard, if there is a way to fix the problem by tuning `snapcraft.yaml`, we had better to use it. However as today it seems that none of us have confidence to do the fix correctly in `snapcraft.yaml`, but the problem really exists and it makes snap users panic when they do upgrade. Now we have 3 easy choices: 1. Tell users to install Gitea snap with `--classic` and use `/usr/bin/gitea` for GITEA_APP_PATH, it makes our snap package not that modern. 2. Replace snap revision path to current path (as above), I prefer this one. 3. Drop snap support, recommend users to use Docker instead of snap. Key problem here is none of us is snap expert, maybe in future we can find a better way.

wxiaoguang reviewed 2021-10-09 16:45:12 +00:00

docs/content/doc/advanced/environment-variables.en-us.md

wxiaoguang commented

2021-10-09 16:45:12 +00:00

Another hacky method:

Every time when gitea starts, if snap environment is detected, make a symlink of current gitea binary to SNAP_DATA directory and use it for git hook config. Then all gitea executable path in hook config all points to SNAP_DATA/gitea, it always points to the current gitea binary. Maybe the symlink step can also be done by snapcraft.yml, then we just need to set GITEA_APP_PATH to SNAP_DATA/gitea on our side https://stackoverflow.com/questions/43505004/how-do-i-target-snap-data-from-snapcraft-yaml

Another hacky method: 4. Every time when gitea starts, if snap environment is detected, make a symlink of current gitea binary to SNAP_DATA directory and use it for git hook config. Then all gitea executable path in hook config all points to SNAP_DATA/gitea, it always points to the current gitea binary. Maybe the `symlink` step can also be done by `snapcraft.yml`, then we just need to set GITEA_APP_PATH to SNAP_DATA/gitea on our side https://stackoverflow.com/questions/43505004/how-do-i-target-snap-data-from-snapcraft-yaml

OleHusgaard (Migrated from github.com) reviewed 2021-10-09 20:44:41 +00:00

docs/content/doc/advanced/environment-variables.en-us.md

OleHusgaard (Migrated from github.com) commented

2021-10-09 20:44:41 +00:00

I have been thinking about this and there may also be another less hacky method:

While we cannot create the right GITEA_APP_PATH environment variable for the runtime environment, probably due to a limitation of how to set the environment variables in snap, we may be able to do so in override-build in snapcraft.yaml. This looks like pure shell scripting, so we could probably set the variable correctly here right before we call make. The makefile could then check if this variable exists, and - if it does - use LDFLAGS to initialize a golang variable appPathOverride at build time. In the beginning of getAppPath() we then simply check if this variable has been set, and return it if this is the case.

I have been thinking about this and there may also be another less hacky method: 5. While we cannot create the right GITEA_APP_PATH environment variable for the runtime environment, probably due to a limitation of how to set the environment variables in snap, we may be able to do so in override-build in snapcraft.yaml. This looks like pure shell scripting, so we could probably set the variable correctly here right before we call make. The makefile could then check if this variable exists, and - if it does - use LDFLAGS to initialize a golang variable appPathOverride at build time. In the beginning of getAppPath() we then simply check if this variable has been set, and return it if this is the case.

wxiaoguang reviewed 2021-10-10 03:55:04 +00:00

docs/content/doc/advanced/environment-variables.en-us.md

wxiaoguang commented

2021-10-10 03:55:04 +00:00

About (5), we can not and should not use LDFLAGS to initialize a golang variable appPathOverride at build time, because the path may change, it can not be predicted at build time.

About (5), we can not and should not `use LDFLAGS to initialize a golang variable appPathOverride at build time`, because the path may change, it can not be predicted at build time.

This repo is archived. You cannot comment on pull requests.

No reviewers