Use LDFLAGS to set the default locations in docker #17501

zeripath · 2021-10-31T09:55:37Z

Too many docker users are caught out by the default location for the
app.ini file being environment dependent so that when they docker exec
into the container the gitea commands do not work properly and require
additional -c arguments to correctly pick up the configuration.

This PR simply builds the default locations into the build.

Fix #14468
Reference #17497
Reference #12082
Reference #8941
... amongst others ...
Replace #17846

Signed-off-by: Andrew Thornton art27@cantab.net

Too many docker users are caught out by the default location for the app.ini file being environment dependent so that when they docker exec into the container the `gitea` commands do not work properly and require additional `-c` arguments to correctly pick up the configuration. This PR simply builds the default locations into the build. Fix go-gitea#14468 Reference go-gitea#17497 Reference go-gitea#12082 Reference go-gitea#8941 ... amongst others ... Signed-off-by: Andrew Thornton <art27@cantab.net>

wxiaoguang · 2021-10-31T11:25:58Z

It makes the docker gitea binary differ from other build.

Can we just use a wrapper to bypass the config problem? eg, we put gitea binary to /usr/local/bin/gitea-real, and make /usr/local/bin/gitea or /app/gitea/gitea a script:

#!/bin/sh
export GITEA_WORK_DIR=/data/gitea
export GITEA_CUSTOM=/data/gitea/custom 
exec /usr/local/bin/gitea-real "$@"

zeripath · 2021-10-31T11:31:56Z

You mean a script like:

https://github.com/go-gitea/gitea/blob/main/contrib/fhs-compliant-script/gitea

wxiaoguang · 2021-10-31T11:35:55Z

Yep, it is more clear, and keeps all Gitea build consistency. My thought is to re-design the directory layout inside docker (Maybe both root-docker and rootless-docker).

For example:

We save gitea binary to /usr/libexec/gitea/gitea, and make a wrapper /usr/local/bin/gitea.

Users can only use the wrapper and they always use the correct environment.

zeripath · 2021-10-31T14:29:07Z

It makes the docker gitea binary differ from other build.

This is already the case.

The binaries in the dockers are built specifically for the docker - they're not copied from the "release" binaries that are made by make release. In particular they have an additional build tag that is not provided to the release binaries. They're built with a different version of go on a different os. They're not even static builds.

Docker

gitea/Dockerfile

Lines 10 to 11 in 4e8a817

    
           ARG TAGS="sqlite sqlite_unlock_notify" 
        
           ENV TAGS "bindata timetzdata $TAGS"

gitea/Dockerfile

Line 23 in 4e8a817

&& make clean-all build

bash-5.1# gitea -v
Gitea version 1.16.0+dev-446-g4e8a81780 built with GNU Make 4.3, go1.17.2 : bindata, timetzdata, sqlite, sqlite_unlock_notify
bash-5.1# ldd $(which gitea)
	/lib/ld-musl-x86_64.so.1 (0x7f76e01db000)
	libc.musl-x86_64.so.1 => /lib/ld-musl-x86_64.so.1 (0x7f76e01db000)

Release Binaries

gitea/Makefile

Lines 634 to 638 in 4e8a817

    
           release-linux: | $(DIST_DIRS) 
        
           	@hash xgo > /dev/null 2>&1; if [ $$? -ne 0 ]; then \ 
        
           		$(GO) install src.techknowlogick.com/xgo@latest; \ 
        
           	fi 
        
           	CGO_CFLAGS="$(CGO_CFLAGS)" xgo -go $(XGO_VERSION) -dest $(DIST)/binaries -tags 'netgo osusergo $(TAGS)' -ldflags '-linkmode external -extldflags "-static" $(LDFLAGS)' -targets '$(LINUX_ARCHS)' -out gitea-$(VERSION) .

$ ~/Downloads/gitea-main-linux-amd64 -v
Gitea version 1.16.0+dev-446-g4e8a81780 built with GNU Make 4.1, go1.16.9 : bindata, sqlite, sqlite_unlock_notify
$ ldd ~/Downloads/gitea-main-linux-amd64
	not a dynamic executable

wxiaoguang · 2021-11-01T09:28:28Z

Then what about the snap and other builds?

techknowlogick · 2021-11-01T16:17:01Z

what about snap and other builds

This change only affects the LDFLAGS for the docker build, in snap and others the binary remains the same (ex, here is snap being built: https://github.com/go-gitea/gitea/blob/main/snap/snapcraft.yaml#L70)

wxiaoguang · 2021-11-01T16:23:37Z

what about snap and other builds

This change only affects the LDFLAGS for the docker build, in snap and others the binary remains the same (ex, here is snap being built: https://github.com/go-gitea/gitea/blob/main/snap/snapcraft.yaml#L70)

I mean, do snap or other builds have similar problems? I know many snap users also meet path & environment problems.

My preference is to make a stable build system, eliminate the differences between builds as much as possible, and never surprise users, if they run gitea xxx they should always be able to use the correct app.ini.

The fhs-compliant-script seems a better solution. https://github.com/go-gitea/gitea/blob/main/contrib/fhs-compliant-script/gitea

About snap, I can see the environment variables like docker:

# https://github.com/go-gitea/gitea/blob/main/snap/snapcraft.yaml
environment:
  GITEA_CUSTOM: "$SNAP_COMMON"
  GITEA_WORK_DIR: "$SNAP_COMMON"
  GIT_TEMPLATE_DIR: "$SNAP/usr/share/git-core/templates"
  GIT_EXEC_PATH: "$SNAP/usr/lib/git-core

zeripath · 2021-11-01T18:50:25Z

The wrapping script would need to be different for each variant - I'm not sure that is really much different from just compiling things in correctly.

(This reminds me though that the FHS script could benefit from a very small improvement... #17513)

wxiaoguang · 2021-11-02T15:35:24Z

Another related issue. Adding LDFLAGS doesn't resolve the design problem fundamentally

delete a user from command line ignores the -c switch with config file #17521

zeripath · 2021-11-02T15:47:36Z

Another related issue. Adding LDFLAGS doesn't resolve the design problem fundamentally

delete a user from command line ignores the -c switch with config file #17521

I think this one is unrelated and is more a problem with: setFlagsAndBeforeOnSubcommands in main.go.

Nope I got this wrong it's app work directory.

But the LDFLAGS would also sort this.

wxiaoguang · 2021-11-02T15:51:22Z

Another related issue. Adding LDFLAGS doesn't resolve the design problem fundamentally

delete a user from command line ignores the -c switch with config file #17521

I think this one is unrelated and is more a problem with: setFlagsAndBeforeOnSubcommands in main.go.

If we can warn users in time when they try to use a wrong app.ini, then the problem can be resolved by user them-selves. We can show a friendly guide message to users to teach them how to find the correct app.ini.

The real problem now is: we seldom warn users clearly, instead gitea tries to do some automatic and "magic" operations for users, which leads to a bigger mistake. This is the fundamental problem in my mind.

But the LDFLAGS would also sort this.

We shouldn't set different LDFLAGS for different builds. We should have a unique and fundamental solution.

zeripath · 2021-11-02T16:19:04Z

The real problem now is: we never warn users, instead gitea tries to do some automatic and "magic" operations for users, which leads to a bigger mistake. This is the fundamental problem in my mind.

Yes, you're preaching to the choir here. I am fully aware that our default paths are not correct for the ways that people use Gitea today.

Gitea was never designed to be dumped in /usr/bin or /usr/local/bin as it is currently compiled. It is currently compiled to do the correct thing if you were to download Gitea and run it from where you downloaded.

We can't change that default because it would be massively breaking for a number of people.

The reality is that no binary you just download from the web would normally be considered to obey the FHS. Things that are meant to obey the FHS usually have to have their source code downloaded and compiled or be in a package.

To that end, I spent a long time creating two options:

The FHS-compliant script
The ability to compile Gitea setting LDFLAGS to compile in the default flags.

Neither of these gained much acceptance - although I understand that Arch uses LDFLAGS.

1. The Filesystem Hierarchy Standard Script

Reasons to use the script:

It's simple and any one can adjust the script to get their defaults as needed
It's clear as to what's happening and the script can remain the same as the binary changes.

Reasons against the script:

You'll need multiple different copies of the script per particular thing to use.
People JUST DON'T USE IT despite it being there for 3 years
If we were to use it on docker we'd have still have to adjust the script.
It may be possible to get out of the exec -a hole to end up writing refs to the actual executable instead of script.
It requires bash.
The user complaints about having to use a script...

It's notable that firefox (at least it used to) required a script like this.

2. LDFLAGS and compiling in the defaults

Reasons to compile things in:

No weird script getting in the way.
The binary is right and looks like any other packaged binary.

Reasons against compiling in:

Each binary is different. Downloading from the website might not work for you anymore.
Each situation we compile for would require a slightly different set of flags and packagers would have to get things correct for their situation.

These two negatives are just the same as any packaged binary though.

My personal suggestion would be that we use LDFLAGS over the script as the script is just not being used. We could provide two different sets of binaries for linux - one obeying the FHS and one with the standard defaults. The docker version would just get the right locations built in.

Snap and other downstream packagers would need to build in the correct paths or use the script as per their own choice.

If you would prefer to use the scripted route then we need to come up with some way to make people use the script.

wxiaoguang · 2021-11-02T16:30:16Z

Maybe we can just improve the gitea's behavior of choosing the app.ini:

Define app.ini candidates: /etc/gitea/app.ini, env defined app.ini, cli defined app.ini
- if more than one of these candidates co-exist, do fatal message to tell user to remove wrong app.ini
Only use FHS wrapper inside docker or snap build for cli
We should fix the gitea's behavior, app.ini should never be created automatically without first gitea web
- If I run gitea -c /tmp/no-such.ini admin user delete --id 123123
- Then I get /tmp/no-such.ini, that's the most strange behavior I meet. That's one of the root cases.

Pros: The problem seems to be resolved. For advanced users, they still can control the behavior. For junior users, no surprise to them, if accident occurs (more candidates), they still get enough message to learn how to resolve it.

Cons: (I can not find a bad case now)

zeripath · 2021-11-02T16:34:42Z

It's not just the app.ini it's the workpath too.

wxiaoguang · 2021-11-02T16:35:11Z

It's not just the app.ini it's the workpath too.

If we can get the correct app.ini, we get the correct work path (or related paths)

lafriks · 2021-11-02T18:22:58Z

For existing instances we can't get correct app.ini anymore except for breaking everything for them by changing behavior. So I agree with @zeripath that using LDFLAGS for docker image gitea binary build would be best solution here

lunny · 2021-11-03T00:52:36Z

For existing instances we can't get correct app.ini anymore except for breaking everything for them by changing behavior. So I agree with @zeripath that using LDFLAGS for docker image gitea binary build would be best solution here

Why we can't ? I think we can get the app.ini correctly according the sequence:

get from flag -c
from ENV $GITEA_CUSTOM/conf/app.ini
/etc/gitea/app.ini if it's UNIX like system
current work directory custom/conf/app.ini

wxiaoguang · 2021-11-03T01:50:36Z

For existing instances we can't get correct app.ini anymore except for breaking everything for them by changing behavior. So I agree with @zeripath that using LDFLAGS for docker image gitea binary build would be best solution here

Why do you think can't we break behaviors? We are releasing new versions, we should fix all strange behaviors.
- I do not think we can't get correct app.ini anymore, my proposal should work without breaking.
How do you resolve the problem (including all non-docker cases) fundamentally?

wxiaoguang · 2021-11-27T11:37:32Z

@lafriks @zeripath

What about my proposal 2: Only use FHS wrapper inside docker or snap build for cli

For example, we put gitea binary in /usr/libexec/gitea-exec, and use a FHS script for /app/gitea/gitea and /usr/local/bin/gitea, the script just do exec /usr/libexec/gitea-exec -c /the-real-app.ini with correct environment variables.

This solution is simple, and doesn't break existing system, and can be used for snap/apt/deb builds.

About concerns:

You'll need multiple different copies of the script per particular thing to use.

That's the same to multiple builds. It doesn't make things worse. That's gitea's problem.

People JUST DON'T USE IT despite it being there for 3 years

Because no one knows it and most people do not know how to use it. For example, I didn't know it before you told me (actually, I was going to write one .... 😂). But we can use it since it works.

If we were to use it on docker we'd have still have to adjust the script.

Yes, as Q1, gitea's strange behavior forces us to make a choice, split the builds, or introduce scripts.

It may be possible to get out of the exec -a hole to end up writing refs to the actual executable instead of script.

Well, I don't know what the real problem is .... some examples for bad cases?

It requires bash.

Not bash, sh is enough. And we already have it, every POSIX system has it.

The user complaints about having to use a script...

They won't know it is a script ..... many Linux programs work this way. And why they complaint .... if they are senior users, they can build their own package and do everything they want. Gitea is not a desktop app.

Too many docker users are caught out by the default location for the app.ini file being environment dependent so that when they docker exec into the container the gitea commands do not work properly and require additional -c arguments to correctly pick up the configuration. This PR simply shadows the gitea binary using variants of the FHS compatible script to make the command gitea have the default locations by default. Fix go-gitea#14468 Reference go-gitea#17497 Reference go-gitea#12082 Reference go-gitea#8941 ... amongst others ... Replace go-gitea#17501 Signed-off-by: Andrew Thornton <art27@cantab.net>

Signed-off-by: Andrew Thornton <art27@cantab.net>

…-in-docker

codecov-commenter · 2021-11-28T20:27:55Z

Codecov Report

Merging #17501 (388307a) into main (9defddb) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main   #17501      +/-   ##
==========================================
+ Coverage   45.54%   45.56%   +0.01%     
==========================================
  Files         809      809              
  Lines       90059    90059              
==========================================
+ Hits        41019    41035      +16     
+ Misses      42484    42473      -11     
+ Partials     6556     6551       -5

Impacted Files	Coverage Δ
models/unit/unit.go	`41.09% <0.00%> (-2.74%)`	⬇️
modules/queue/queue_channel.go	`95.00% <0.00%> (-1.67%)`	⬇️
models/issue_comment.go	`52.69% <0.00%> (+0.28%)`	⬆️
models/notification.go	`65.65% <0.00%> (+0.86%)`	⬆️
modules/notification/ui/ui.go	`62.31% <0.00%> (+1.44%)`	⬆️
modules/queue/workerpool.go	`53.05% <0.00%> (+1.90%)`	⬆️
modules/notification/mail/mail.go	`38.23% <0.00%> (+2.94%)`	⬆️
models/gpg_key_common.go	`64.51% <0.00%> (+4.83%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9defddb...388307a. Read the comment docs.

Too many docker users are caught out by the default location for the app.ini file being environment dependent so that when they docker exec into the container the gitea commands do not work properly and require additional -c arguments to correctly pick up the configuration. This PR simply shadows the gitea binary using variants of the FHS compatible script to make the command gitea have the default locations by default. Fix #14468 Reference #17497 Reference #12082 Reference #8941 ... amongst others ... Replace #17501 Signed-off-by: Andrew Thornton <art27@cantab.net>

zeripath · 2021-12-01T18:23:06Z

Closed in preference of #17846

Too many docker users are caught out by the default location for the app.ini file being environment dependent so that when they docker exec into the container the gitea commands do not work properly and require additional -c arguments to correctly pick up the configuration. This PR simply shadows the gitea binary using variants of the FHS compatible script to make the command gitea have the default locations by default. Fix go-gitea#14468 Reference go-gitea#17497 Reference go-gitea#12082 Reference go-gitea#8941 ... amongst others ... Replace go-gitea#17501 Signed-off-by: Andrew Thornton <art27@cantab.net>

zeripath added topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile theme/docker labels Oct 31, 2021

zeripath added this to the 1.16.0 milestone Oct 31, 2021

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 31, 2021

techknowlogick approved these changes Oct 31, 2021

View reviewed changes

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 Oct 31, 2021

techknowlogick and others added 3 commits November 18, 2021 03:28

Merge branch 'main' into set-default-locations-in-docker

374ea87

Merge branch 'main' into set-default-locations-in-docker

493a4ee

Merge branch 'main' into set-default-locations-in-docker

fcfce5a

wxiaoguang mentioned this pull request Nov 23, 2021

Improve install code to avoid low-level mistakes. #17779

Merged

zeripath mentioned this pull request Nov 28, 2021

Use shadowing script for docker #17846

Merged

zeripath added 2 commits November 28, 2021 19:33

fix bug in fhs script

b3df346

Signed-off-by: Andrew Thornton <art27@cantab.net>

Merge remote-tracking branch 'origin/main' into set-default-locations…

388307a

…-in-docker

zeripath closed this Dec 1, 2021

zeripath deleted the set-default-locations-in-docker branch December 1, 2021 18:23

zeripath mentioned this pull request Jan 22, 2022

Using exec -a in gitea/gitea:dev container broke SSH passthrough access #18117

Closed

go-gitea locked and limited conversation to collaborators Apr 28, 2022

delvh added topic/distribution This PR changes something about the packaging of Gitea and removed theme/docker labels Oct 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use LDFLAGS to set the default locations in docker #17501

Use LDFLAGS to set the default locations in docker #17501

zeripath commented Oct 31, 2021 •

edited

Loading

wxiaoguang commented Oct 31, 2021

zeripath commented Oct 31, 2021

wxiaoguang commented Oct 31, 2021 •

edited

Loading

zeripath commented Oct 31, 2021

wxiaoguang commented Nov 1, 2021

techknowlogick commented Nov 1, 2021

wxiaoguang commented Nov 1, 2021 •

edited

Loading

zeripath commented Nov 1, 2021 •

edited

Loading

wxiaoguang commented Nov 2, 2021

zeripath commented Nov 2, 2021 •

edited

Loading

wxiaoguang commented Nov 2, 2021 •

edited

Loading

zeripath commented Nov 2, 2021

wxiaoguang commented Nov 2, 2021 •

edited

Loading

zeripath commented Nov 2, 2021

wxiaoguang commented Nov 2, 2021 •

edited

Loading

lafriks commented Nov 2, 2021

lunny commented Nov 3, 2021

wxiaoguang commented Nov 3, 2021 •

edited

Loading

wxiaoguang commented Nov 27, 2021 •

edited

Loading

codecov-commenter commented Nov 28, 2021

zeripath commented Dec 1, 2021

Use LDFLAGS to set the default locations in docker #17501

Use LDFLAGS to set the default locations in docker #17501

Conversation

zeripath commented Oct 31, 2021 • edited Loading

wxiaoguang commented Oct 31, 2021

zeripath commented Oct 31, 2021

wxiaoguang commented Oct 31, 2021 • edited Loading

zeripath commented Oct 31, 2021

Docker

Release Binaries

wxiaoguang commented Nov 1, 2021

techknowlogick commented Nov 1, 2021

wxiaoguang commented Nov 1, 2021 • edited Loading

zeripath commented Nov 1, 2021 • edited Loading

wxiaoguang commented Nov 2, 2021

zeripath commented Nov 2, 2021 • edited Loading

wxiaoguang commented Nov 2, 2021 • edited Loading

zeripath commented Nov 2, 2021

1. The Filesystem Hierarchy Standard Script

2. LDFLAGS and compiling in the defaults

wxiaoguang commented Nov 2, 2021 • edited Loading

zeripath commented Nov 2, 2021

wxiaoguang commented Nov 2, 2021 • edited Loading

lafriks commented Nov 2, 2021

lunny commented Nov 3, 2021

wxiaoguang commented Nov 3, 2021 • edited Loading

wxiaoguang commented Nov 27, 2021 • edited Loading

codecov-commenter commented Nov 28, 2021

Codecov Report

zeripath commented Dec 1, 2021

zeripath commented Oct 31, 2021 •

edited

Loading

wxiaoguang commented Oct 31, 2021 •

edited

Loading

wxiaoguang commented Nov 1, 2021 •

edited

Loading

zeripath commented Nov 1, 2021 •

edited

Loading

zeripath commented Nov 2, 2021 •

edited

Loading

wxiaoguang commented Nov 2, 2021 •

edited

Loading

wxiaoguang commented Nov 2, 2021 •

edited

Loading

wxiaoguang commented Nov 2, 2021 •

edited

Loading

wxiaoguang commented Nov 3, 2021 •

edited

Loading

wxiaoguang commented Nov 27, 2021 •

edited

Loading