Propose the ability to use podman #42
Conversation
metal3d
force-pushed
the
propose-podman-fixes
branch
from
79b6542
to
d219030
Compare
|
Thanks for the PR it looks good to me, just few thoughts. PRs should be have The GOFLAGS could be also set via the fyne-cross |
|
I changed the base, so there is a conflict I will resolve. I will check for GOFLAG. |
metal3d
force-pushed
the
propose-podman-fixes
branch
from
d219030
to
0580aeb
Compare
|
Sorry there were a non fixed conflict, I will resolve (and fix the test) |
|
@lucor I made a new test to fix GOFLAGS if it is sent in parameters. I can see: And with |
There was a problem hiding this comment.
I've tried the changes using a Version as per your example.
I was able to see it into the logs, but in the fyne app that uses that variable seems to be ignored :(
Wondering if the multiple -ldflags are allowed.
Additionally I forgot that there is a fyne-cross --ldflags that we should handle as well.
internal/command/docker.go
Outdated
| // findGoFlags return the index of context where GOFLAGS is set. | ||
| func findGoFlags(ctx Context) (int, error) { | ||
| for i, env := range ctx.Env { | ||
| if strings.Index(env, "GOFLAGS") == 0 { | ||
| return i, nil | ||
| } | ||
| } | ||
| return -1, errors.New("GOFLAGS not found in context") | ||
| } |
There was a problem hiding this comment.
| // findGoFlags return the index of context where GOFLAGS is set. | |
| func findGoFlags(ctx Context) (int, error) { | |
| for i, env := range ctx.Env { | |
| if strings.Index(env, "GOFLAGS") == 0 { | |
| return i, nil | |
| } | |
| } | |
| return -1, errors.New("GOFLAGS not found in context") | |
| } | |
| // findGoFlags return the index of context where GOFLAGS is set, or -1 if is not present. | |
| func findGoFlags(ctx Context) int { | |
| for i, env := range ctx.Env { | |
| if strings.HasPrefix(env, "GOFLAGS") { | |
| return i | |
| } | |
| } | |
| return -1 | |
| } |
There was a problem hiding this comment.
What about not returning an error here ?
There was a problem hiding this comment.
Also, in the same part do the file. Couldn’t we just use strings.Contains?
There was a problem hiding this comment.
@lucor I did it the "go" way, but yes, it's possible to only return the index that is -1 if not found
@Jacalz because I want to be sure that this is the environment variable that is found and nothing more. It is possible that you want to pass "GOFLAGS" as a string in -X options (for example to inject a message in a variable at build time to say "with GOFLAGS")
There was a problem hiding this comment.
@lucor I don't understand your first question. I was able to set multiple -ldflags - to set Version I also made the test and that worked with: -e GOFLAGS=-ldflags=-X='main.Version=1.0.0'
There was a problem hiding this comment.
I did it the "go" way, but yes, it's possible to only return the index that is -1 if not found
Thanks for the changes. I see and agree with you about the Go way to return the error. In that specific case I'm ok with a little exception in favour of saving an err check like the strings.Index method due the fact it won't return error.
don't understand your first question. I was able to set multiple -ldflags - to set Version I also made the test and that worked with: -e GOFLAGS=-ldflags=-X='main.Version=1.0.0'
I tested locally with a small fyne app and tried to "inject" to variable values with ldflags but despite the fact I was able to see the flags in the fyne-cross compilation output, it was not set for the app.
Code I've used to test:
cat main.go
package main
import (
"fyne.io/fyne/v2"
"fyne.io/fyne/v2/app"
"fyne.io/fyne/v2/container"
"fyne.io/fyne/v2/widget"
)
var Title = "Default"
var Name = "Default"
func main() {
a := app.New()
w := a.NewWindow(Title)
hello := widget.NewLabel("Hi " + Name)
w.SetContent(container.NewVBox(
hello,
))
w.Resize(fyne.NewSize(200, 200))
w.ShowAndRun()
}
Command used to compile:
fyne-cross linux --env GOFLAGS="-ldflags=-X='main.Name=Podman'" --debug .
fyne-cross logs:
[i] Building binary...
/usr/bin/docker run --rm -t -w /app -v /code/lucor/test:/app:z -v /home/lucor/.cache/fyne-cross:/go:z -e CGO_ENABLED=1 -e GOCACHE=/go/go-build -e GOFLAGS=-ldflags=-X='main.Name=Podman' -ldflags=-w -ldflags=-s -e GOOS=linux -e GOARCH=amd64 -e CC=gcc -e fyne_uid=1000 fyneio/fyne-cross:1.1-base go build -o /app/fyne-cross/bin/linux-amd64/test -v .
[✓] Binary: /code/lucor/test/fyne-cross/bin/linux-amd64/test
[i] Packaging app...
/usr/bin/docker run --rm -t -w /app -v /code/lucor/test:/app:z -e CGO_ENABLED=1 -e GOCACHE=/go/go-build -e fyne_uid=1000 fyneio/fyne-cross:1.1-base /usr/local/bin/fyne version
fyne cli version v2.0.2
/usr/bin/docker run --rm -t -w /app/fyne-cross/tmp/linux-amd64 -v /code/lucor/test:/app:z -v /home/lucor/.cache/fyne-cross:/go:z -e CGO_ENABLED=1 -e GOCACHE=/go/go-build -e GOFLAGS=-ldflags=-X='main.Name=Podman' -ldflags=-w -ldflags=-s -e GOOS=linux -e GOARCH=amd64 -e CC=gcc -e fyne_uid=1000 fyneio/fyne-cross:1.1-base /usr/local/bin/fyne package -os linux -name test -icon /app/fyne-cross/tmp/linux-amd64/Icon.png -appBuild 1 -appVersion 1.0 -executable /app/fyne-cross/bin/linux-amd64/test
[✓] Package: /code/lucor/test/fyne-cross/dist/linux-amd64/test.tar.gz
Application output:
Please also notes, that fyne-cross exposes also a -ldflags so we should have the following command to work too:
fyne-cross linux -ldflags="-X=main.Name=Podman" --debug .
Wrt the multilple ldflags, it seems that when you specify them like:
GOFLAGS="-ldflags=-X='main.Name=Podman' -ldflags=-s -ldflags=-w" go build
or
go build -ldflags=-X='main.Name=Podman' -ldflags=-s -ldflags=-w
only the last flag is used (try to compile changing the order)
The following instead work as expected:
go build -ldflags "-X=main.Name=Podman -w -s"
There was a problem hiding this comment.
Thanks again for working on this @metal3d. It is a very nice feature to have and would be nice to see it landed soon. Do you think you could get this rebased on develop? The tests don't want to start and I think the out-of-date branch might partly be the reason.
I'm currently makeing the merges and conflct resolution. I hope to not make regression...
I finally managed to test this out on my new Fedora 34 installation. I had to install
podman-dockerto even get it started. Would it be possible to get it to only usepodman? After that, I ran into this error:
This is what I discussed earlier - I wanted to use the "podman" command instead of "docker" with "podman-docker" mackage. Yes, of course, the best is to use the "podman" command IMHO. I can make some changes for this.
~/go/bin/fyne-cross linux -arch amd64,arm64 -app-id com.github.jacalz.wormhole-gui -icon internal/assets/icon/icon-512.png [i] Target: linux/amd64 [i] Cleaning target directories... [✓] "bin" dir cleaned: /home/jacob/Git/wormhole-gui/fyne-cross/bin/linux-amd64 [✓] "dist" dir cleaned: /home/jacob/Git/wormhole-gui/fyne-cross/dist/linux-amd64 [✓] "temp" dir cleaned: /home/jacob/Git/wormhole-gui/fyne-cross/tmp/linux-amd64 [i] Checking for go.mod: /home/jacob/Git/wormhole-gui/go.mod [✓] go.mod found [i] Building binary... USE PODMAN Emulate Docker CLI using podman. Create /etc/containers/nodocker to quiet msg. Error: error getting default registries to try: short-name resolution enforced but cannot prompt without a TTY [✗] exit status 125 make: *** [Makefile:51: linux] Error 1
It's been a while I didn't make tests with my fyne-cross changes, I don't remember how I tested 😅
I will try to make some tests a soon as the confict and merge / rebase are OK
There was a problem hiding this comment.
OK, I see the problem.
This is the test file:
package main
import (
"fyne.io/fyne/v2/app"
"fyne.io/fyne/v2/widget"
)
var Message = "Default"
func main() {
app := app.New()
w := app.NewWindow(Message)
t := widget.NewLabel(Message)
w.SetContent(t)
w.ShowAndRun()
}When I build with:
../../fyne-cross/fyne-cross linux -ldflags="-X main.Message=42"
The error is:
go: parsing $GOFLAGS: non-flag "main.Message=42"
If I do:
# note the "=" after -X
../../fyne-cross/fyne-cross linux -ldflags="-X=main.Message=42"
So it builds, but the main.Message is not set...
it seems to be related to golang/go#26849
There was a problem hiding this comment.
OK... I have new information.
The documentation says:
GOFLAGS
A space-separated list of -flag=value settings to apply
to go commands by default, when the given flag is known by
the current command. Each entry must be a standalone flag.
Because the entries are space-separated, flag values must
not contain spaces. Flags listed on the command line
are applied after this list and therefore override it.
So, "flags value" cannot contain spaces... And -ldflag=-X main.Value=foo is badly interpreted. This is the only one ldflag that will not work with -X=main...
That means that this particular ldflag shoud be passed in the command line, not in environment.
So, what I can try is to pass ldflags to the command line...
There was a problem hiding this comment.
GOFLAGS seems to be fixed in aa8002d
I didn't tried with "docker". Note that now, the "podman" command is used even if "docker" command is an alias
internal/command/docker.go
Outdated
| } | ||
| flags := make([]string, len(ldflags)) | ||
| for i, flag := range ldflags { | ||
| flags[i] = fmt.Sprintf("-ldflags=%s", flag) |
There was a problem hiding this comment.
You should be able to just as well not use fmt.Sprintf. Just do flags[i] = "-ldflags=" + flag. Just as readable (in my opinion) but much faster.
There was a problem hiding this comment.
Thanks again for working on this @metal3d. It is a very nice feature to have and would be nice to see it landed soon. Do you think you could get this rebased on develop? The tests don't want to start and I think the out-of-date branch might partly be the reason.
I finally managed to test this out on my new Fedora 34 installation. I had to install podman-docker to even get it started. Would it be possible to get it to only use podman? After that, I ran into this error:
~/go/bin/fyne-cross linux -arch amd64,arm64 -app-id com.github.jacalz.wormhole-gui -icon internal/assets/icon/icon-512.png
[i] Target: linux/amd64
[i] Cleaning target directories...
[✓] "bin" dir cleaned: /home/jacob/Git/wormhole-gui/fyne-cross/bin/linux-amd64
[✓] "dist" dir cleaned: /home/jacob/Git/wormhole-gui/fyne-cross/dist/linux-amd64
[✓] "temp" dir cleaned: /home/jacob/Git/wormhole-gui/fyne-cross/tmp/linux-amd64
[i] Checking for go.mod: /home/jacob/Git/wormhole-gui/go.mod
[✓] go.mod found
[i] Building binary...
USE PODMAN
Emulate Docker CLI using podman. Create /etc/containers/nodocker to quiet msg.
Error: error getting default registries to try: short-name resolution enforced but cannot prompt without a TTY
[✗] exit status 125
make: *** [Makefile:51: linux] Error 1
|
I will soon. Sorry for the delay, very busy these days...
|
I made some changes to detect docker, and if "docker" is a "podman" alias. So I can use "docker" or "podman" command now. I want to fix the ldflags problem before. |
OK, for the registry, now, Fedora proposes several registries. The image location should be "docker.io/blablabla" for "moby-engine" (the docker distribution by RH/Fedora" and podman. I should not have this problem because I built the image myself (to fix the entrypoint for podman) So, the image "url" should be changed to its complete form. |
There was a problem hiding this comment.
Sorry for the late review. I've had a lot to do with work related stuff.
I still could not use only podman, had to install podman-docker to get to this point. If it is possible to egt it to only use podman directly, that would be great, but not a necessity, I guess. This is looking better, but I am still getting the following output when trying to use it:
/home/jacob/go/bin/fyne-cross windows -arch amd64 -app-id io.github.jacalz.seaborne
[i] Target: windows/amd64
[i] Cleaning target directories...
[✓] "bin" dir cleaned: /home/jacob/Git/seaborne/fyne-cross/bin/windows-amd64
[✓] "dist" dir cleaned: /home/jacob/Git/seaborne/fyne-cross/dist/windows-amd64
[✓] "temp" dir cleaned: /home/jacob/Git/seaborne/fyne-cross/tmp/windows-amd64
[i] Checking for go.mod: /home/jacob/Git/seaborne/go.mod
[✓] go.mod found
Error: short-name resolution enforced but cannot prompt without a TTY
[✗] could not package the Fyne app: exit status 125
You should not require to install "podman-docker", I haven't it and it works now with "podman" command if needed. For the "short" link name, this is a problem. As explained, podman (and the "docker" version of Fedora named "moby-engine") proposes to use others registries, so we need TTY to answer. If the image is already present (for example because I compiled the image myself to test my changes) so it works. The problem is that it seems you don't have the image on your Fedora (and it's absolutely normal) - The "solution" is: we change the image path to "docker.io/fyne/fyne-cross" ("docker.io" added to the image name, so podman or moby-engine understand where is the repository) - but this has got a bad behaviour: we can't use a locally built image... I'm trying to find a way to make it possible to have a TTY to answer the question asked by podman/moby... this problem doesn't happen if you use "docker" (because they configured the docker client to use "docker.io" by default). Or, maybe you have a better idea ? |
|
@Jacalz I tried and see this: and the image is well pulled, but the "pullImage" seems to not being used at this stage. But, for me, it never asks for TTY... Are you up to date with podman ? |
There was a problem hiding this comment.
Thanks for your work on this. the latest changes seems to make it mostly work without installing podman-docker, nice work. Looks like a few changes are part of this PR that are unrelated to this. Did the merge go wrong?
I'm still running into the issue with TTY on my fully updated Fedora 34 installation. Am I missing something with how I should use podman? I literally just used what I had installed, no tweaking or other changes done on my part.
I also tried doing fyne-cross windows -pull and got an error that the docker executable wasn't available. I tried installing podman-docker and got this instead:
[✓] go.mod found
chown: changing ownership of '/go': Operation not permitted
[✗] could not package the Fyne app: exit status 1
Got a similar error when I tried to run a regular build using fyne-cross as well after having installed podman-docker.
|
Yes. You need to build images because I made some changes on entrypoint.
With podman, there is no need to change owner because I'm using the
"keepid" option. So, for podman, the new entrypoint skips this stage.
For tty, I will try to understand why there is a different behavior on my
laptop. I didn't tweak anything. I removed the images and it pull images
whitout trying to ask me the registry to use. So maybe there's a cache on
previous answers...
I will try to reset everything on my podman installation to reproduce the
problem.
Le jeu. 8 juil. 2021 à 21:24, Jacob ***@***.***> a écrit :
… ***@***.**** requested changes on this pull request.
Thanks for your work on this. the latest changes seems to make it mostly
work without installing podman-docker, nice work. Looks like a few
changes are part of this PR that are unrelated to this. Did the merge go
wrong?
I'm still running into the issue with TTY on my fully updated Fedora 34
installation. Am I missing something with how I should use podman? I
literally just used what I had installed, no tweaking or other changes done
on my part.
I also tried doing fyne-cross windows -pull and got an error that the
docker executable wasn't available. I tried installing podman-docker and
got this instead:
[✓] go.mod found
chown: changing ownership of '/go': Operation not permitted
[✗] could not package the Fyne app: exit status 1
Got a similar error when I tried to run a regular build using fyne-cross
as well after having installed podman-docker.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#42 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAYN4HFNZCH3M6BRPOLQ5TTWX3PLANCNFSM43MKE6KQ>
.
|
|
I did use it thorough a Makefile if I remember correctly. It shouldn't cause any issues, I think, but I'll try without just to be sure. |
There were something weird as soon as I pull "develop" branch then rebased mine. I don't know why but some things were "recommitted" without any reason. For example, there is a problem with CHANGELOG.md file... whatever I do, there is a difference between mine and the one from develop branch... |
metal3d
force-pushed
the
propose-podman-fixes
branch
from
b6218d6
to
cee18b0
Compare
|
I made fixes on the rebase, it seems that it's now a bit better. |
|
OK @Jacalz I see the problem. My laptops have this file: (You can check if your configuration is changed from the RPM version Inside, there is a list of When TTY is not accessible, podman searches inside these registries - but not on your laptop. It's weird because I made tests on my 2 laptops and both don't ask anything. But, if you've got the problem, so others will have the same problem. There are 2 possibilities:
The second option is better IMHO, but to needs a bit more modifications. I can do it. Do you want me to make this changes ? |
|
@Jacalz I found a way to avoid the problem in a88ea79
So:
This fixes problems for me. It works with Docker and Podman. Note that OCI recommends using the full image name with domain since there are now a few public registries, using the full name avoids image spoofing. |
There was a problem hiding this comment.
Thanks for the great work with the changes. I still need to test on my workstation, but I just tested on my laptop (of which I had never used docker or podman). I downloaded this branch and just ran the following command and got the below output.This is getting very close now :)
[jacob@fedora seaborne]$ make windows
/home/jacob/go/bin/fyne-cross windows -arch amd64 -app-id io.github.jacalz.seaborne
[i] Target: windows/amd64
[i] Cleaning target directories...
[✓] "temp" dir cleaned: /home/jacob/Git/seaborne/fyne-cross/tmp/windows-amd64
[✓] "bin" dir cleaned: /home/jacob/Git/seaborne/fyne-cross/bin/windows-amd64
[✓] "dist" dir cleaned: /home/jacob/Git/seaborne/fyne-cross/dist/windows-amd64
[i] Checking for go.mod: /home/jacob/Git/seaborne/go.mod
[✓] go.mod found
Trying to pull docker.io/fyneio/fyne-cross:1.1-windows...
Getting image source signatures
Copying blob 65d943ee54c1 done
Copying blob 8962bc0fad55 done
Copying blob e8d62473a22d done
Copying blob d960726af2be done
Copying blob 186c77a2a533 done
Copying blob f2253e6fbefa done
Copying blob db807893dccf done
Copying blob 292f24f0ff89 done
Copying blob 974bf8135e20 done
Copying blob 6f6e9bee4d1e done
Copying blob bf3ad36f9777 done
Copying blob 2411a53787f3 done
Copying config dcf827d6fe done
Writing manifest to image destination
Storing signatures
chown: changing ownership of '/go': Operation not permitted
[✗] could not package the Fyne app: exit status 1
make: *** [Makefile:28: windows] Error 1
- Only return the index - Use `strings.HasPrefix` instead of `strings.Index`
- fix the ldflags to be set in GOFLAGS, so there is no problem by escaping environment variables - ldflags "-H" should be set with equal sign `-H=windowsgui` instead of `-H windowsgui' - add podman detection - add podman usage: no need to fix uid with gosu, podman has got an option named "--userns" to set to "keep-id".
- Only return the index - Use `strings.HasPrefix` instead of `strings.Index`
- fix the ldflags to be set in GOFLAGS, so there is no problem by escaping environment variables - ldflags "-H" should be set with equal sign `-H=windowsgui` instead of `-H windowsgui' - add podman detection - add podman usage: no need to fix uid with gosu, podman has got an option named "--userns" to set to "keep-id".
- Only return the index - Use `strings.HasPrefix` instead of `strings.Index`
- Better podman detection, because the user could not have podman-socket so we detect if "docker" command is installed, and if it's an alias - either we can use "podman" command - The podman detection is now cached, because we need to have this information several times - Fix the "ldflags" detection. The "-X" option for ldflags is a special case that we can pass to the command line argument. There are errors if it's passed in GOFLAGS
- The runner is now globally set - We prefix the image by "docker.io" in calls. It works with any runner (docker, podman, runc...) - it's recommanded by OCI and it avoids podman to ask which registry to use. - If podman is used, the Makefile tags the images with the "docker.io" prefix. It could be OK for docker also, but at this time it's enough to avoid the image overwrite
We need to parse the entire ldflag argument to check if there are several `-X` parameters. Then we rebuild the entire string. TODO: This need automated tests.
metal3d
force-pushed
the
propose-podman-fixes
branch
from
ce8e9fe
to
ad1b9d7
Compare
This will help to be compatible with others engines later.
|
@metal3d Was I supposed to do a re-review on this? I was never requested for a re-review but it looks like you have pushed a few commits since the last time. If I was, I'm sorry that it took me three months to reach to this :I |
| ) | ||
|
|
||
| // CheckRequirements checks if the docker binary is in PATH | ||
| func CheckRequirements() error { | ||
| _, err := execabs.LookPath("docker") | ||
| _, err := exec.LookPath("docker") |
There was a problem hiding this comment.
Could you please switch back to execabs (applies to more places in this PR)? It is being used because exec has a security issue on Windows that can't be fixed without breaking the API stability guarantee.
|
Thanks @metal3d and sorry for the long wait. |
|
I'm sorry to not have answered for a while. Many things happened last months and I missed a lot of discussion. I'm very happy you accepted this PR :) |
|
No worries. Thanks for all your hard work! |
|
You can't use it just yet with as the updated images aren't on docker hub currently (I think so at least). They need to be tagged differently, so you'll either have to wait for new images or run EDIT: The question seems to have been moved to the issue instead. |
escaping environment variables
-H=windowsguiinstead of`-H windowsgui'
option named "--userns" to set to "keep-id".