fyne-io / fyne-cross

Cross compiler tool for Fyne apps
BSD 3-Clause "New" or "Revised" License
237 stars 48 forks source link

fyne-cross option -ldflags does not work properly like it does in go build #190

Closed vinser closed 4 months ago

vinser commented 1 year ago

Describe the bug:

Unlike go build fyne-cross option ldflags does nothing when I try to set global var with ldflags "-X 'main.ver=1.0.0'"

To Reproduce:

For the code beneath - steps to reproduce the behavior:

  1. Run the command fyne-cross linux -ldflags "-X 'main.version=0.0.1'"

  2. Run executable was built

  3. See no version is displayed image

  4. Run the same code with the command go run -ldflags "-X 'main.version=0.0.1'" .

  5. See version is displayed image

Example code:


package main

import (
    "fyne.io/fyne/v2"
    "fyne.io/fyne/v2/app"
    "fyne.io/fyne/v2/widget"
)

var version string

func main() {
    a := app.New()
    w := a.NewWindow("Hello World")
    w.Resize(fyne.NewSize(200, 100))

    w.SetContent(widget.NewLabel("Version: " + version))
    w.ShowAndRun()
}

Device and debug info (please complete the following information):

Device info
andydotxyz commented 1 year ago

BTW it is recommended to use metadata rather than LDFlags as they do not work on all platforms even if you can get the commands into the compiler https://developer.fyne.io/started/metadata

vinser commented 1 year ago

Thank you for the info about go problems with ldflags on some platforms but on linux and windows desktop OSes it works. I've used -ldflags in my other not fyne go project and it succeeded.

When it comes to using metadata I know that vars are initialized with fyne and fyne-cross by means of transient go file with init() func. And if I do trivial go run . metadata vars will stay uninitialized. With -ldfalgs I should get right inited vars in both cases.

I didn't found in fyne docs they recommended mandatory use of metadata or FyneApp.toml. If there is no way to fix the ldflags option in fyne-cross it will be right to delete ldflags option from fyne-cross help command to eliminate issues :)

It's a real bug

Bluebugs commented 1 year ago

From memory, I think fyne-cross follow GOFLAGS syntax for ldflags. So the command line should be something like fyne-cross linux -ldflags=-X -ldflags=main.version=0.0.1.

Bluebugs commented 1 year ago

It also means you can set those argument in GOFLAGS directly before calling fyne-cross. So something like: GOFLAGS="-ldflags=-X -ldflags=main.version=0.0.1" fyne-cross linux should also work the same.

vinser commented 1 year ago

In fyne-cross sources I found such a test case:

        {
            name: "custom ldflags",
            args: args{
                flags: &CommonFlags{
                    AppBuild: 1,
                    Ldflags:  "-X main.version=1.2.3",
                },
            },
            want: Context{
                AppBuild:     "1",
                AppID:        "",
                Volume:       vol,
                CacheEnabled: true,
                StripDebug:   true,
                Package:      ".",
                Engine:       engine,
                Env: map[string]string{
                    "GOFLAGS": "-ldflags=-X -ldflags=main.version=1.2.3",
                },
            },
            wantErr: false,
        },

and it passed May be it's a bug in docker container?

vinser commented 1 year ago

GOFLAGS="-ldflags=-X -ldflags=main.version=0.0.1" fyne-cross linux makes the same bug result

Bluebugs commented 1 year ago

I had a better look at this and it is a bit weird. It seems something is happening when calling fyne package with the -release inside the container when fyne-cross call it. Manual call work, but fyne-cross call doesn't. I do not understand yet.

We also do not pick the OS GOFLAGS and I have created a PR for it.

Finally the syntax is the following at the moment (before my PR): fyne-cross linux -ldflags=-X=main.version=0.0.1

After my PR the following, which follow actually go expectation: GOFLAGS="-ldflags=-X=main.version=0.0.1" fyne-cross linux

vinser commented 1 year ago

The syntax you describe above fyne-cross linux -ldflags=-X=main.version=0.0.1 does not work too - main.version is not initialized.

Concerning -release option with -ldflags I think it's because fyne has no -ldflags option for release command now when I run fyne release -help unlike fine build -help

Bluebugs commented 1 year ago

Sorry, that line should be working, but can't figure out why. The line that work for me is: fyne-cross linux -ldflags=-X=main.version=0.0.1 -no-strip-debug which remove the -release from the call to fyne package not to be mixed with fyne release. You are right about fyne release that was missed, but we use GOFLAGS to set the ldflags for the call to fyne at the moment. That should work.

The thing that change in fyne build step, it is parsing the GOFLAGS and trying to insert the -w -s as needed. Somehow this seems to mess up with the GOFLAGS we have set, but only when calling via fyne-cross. As using the container manually with the same command work... As I say, I don't know what is going here, maybe I am not noticing a typo somewhere or something like that...

vinser commented 1 year ago

I found that command GOFLAGS=-ldflags="-X=main.version=0.0.1" fyne package -release sets main.version var right to 0.0.1 value like go build -ldflags "-X 'main.version=0.0.1'" . does. But GOFLAGS=-ldflags="-X=main.version=0.0.1" fyne-cross linux does not. This is because GOFLAGS are not passed into container when fyne-cross runs fyne in container. Below is the line with call string from output of GOFLAGS=-ldflags="-X=main.version=0.0.1" fyne-cross linux -debug 2>build.log


/usr/bin/docker run --rm -t -w /app -v /home/vinser/Dev/t_fyne:/app:z -v /home/vinser/.cache/fyne-cross:/go:z --platform linux/arm64 -u 1000:1000 --entrypoint fixuid -v /tmp/ssh-XXXXXXq4THxC/agent.43703:/tmp/ssh-agent -e SSH_AUTH_SOCK=/tmp/ssh-agent -e CGO_ENABLED=1 -e GOCACHE=/go/go-build -e CXX=zig c++ -target aarch64-linux-gnu -isystem /usr/include -L/usr/lib/aarch64-linux-gnu -e GOOS=linux -e GOARCH=arm64 -e CC=zig cc -target aarch64-linux-gnu -isystem /usr/include -L/usr/lib/aarch64-linux-gnu docker.io/fyneio/fyne-cross-images:linux /usr/local/bin/fyne package -os linux -name t_fyne -icon /app/fyne-cross/tmp/linux-arm64/Icon.png -appBuild 20 -appVersion 1.0.0 -appID com.github.vinser.t_fyne -release

Bluebugs commented 1 year ago

Yes, that's why I did open PR #193 . The things that should currently work is: fyne-cross linux -ldflags=-X=main.version=0.0.1 -no-strip-debug

pkk-code commented 1 year ago

May i know when this would be fixed?

andydotxyz commented 1 year ago

It seems that there is a PR open, so you can see the latest over there.

PhanSon95 commented 10 months ago

I got similar bug with ldflag too Do you have any solution?

andydotxyz commented 10 months ago

Did you see the PR linked above?

richie-tt commented 8 months ago

Any progress ??

I can never understand why a project owner dooms his project to abandonment.

Someone wanted to help you and solve the problem, as the community always does. And Yes I see that you asking about the "unit tests" for this PR, but not everyone is the best developer, you should help him instead of punishing him for that he wanted to help you.

I hope I am wrong about you intention

richie-tt commented 8 months ago

And I have the same problem; for now, I am fixing it using the sed command. I know this is hacky but it works.

andydotxyz commented 8 months ago

And Yes I see that you asking about the "unit tests" for this PR, but not everyone is the best developer, you should help him instead of punishing him for that he wanted to help you.

I'm not quite sure what the intent of your message is here. Cedric who I was discussing this with is a core contributor to the project. Note also that the PR has been accepted - so he could merge it if he believed that I was wrong in the push back.

Nobody is "dooming this project to abandonment" and this is run by a group of people, not a single individual.

williambrode commented 4 months ago

I believe this is a regression in 1.4.0. I updated from 1.3.0 (and my go version at the same time) and this broke my versioning which now means many customers auto-updates won't function and I'll have to have them manually re-download. Should there be some kind of notice on the readme to let people know about the regression?

To be clear on the status - the PR that was merged only allows GOFLAGS to be used as a workaround? So the issue still persists I believe? I may take a stab at it since I'd like to get this working again in my CI and I can't downgrade to 1.3.0 due to my go version.

andydotxyz commented 4 months ago

The issue has been resolved as far as I can see. Please try the 1.5.0 release which includes the change.

williambrode commented 4 months ago

There is still more work to be done here. The test in fyne-cross is validating the wrong behavior (tested with go 1.22.3):

PS C:\Projects\gotest> $Env:GOFLAGS="-ldflags=-X -ldflags=example.com/m/main.Version=0.0.1"
PS C:\Projects\gotest> go build
# example.com/m                                                                                                 
C:\Program Files\Go\pkg\tool\windows_amd64\link.exe: -X flag requires argument of the form importpath.name=value

I also verified it will silently fail (second flag overwrites the first) when its not a syntax error:

PS C:\Projects\gotest> $Env:GOFLAGS="-ldflags=-X=main.Version=0.0.1"                       
PS C:\Projects\gotest> go build                                     
PS C:\Projects\gotest> ./m.exe                                      
Version1: 0.0.1, Version2: dev
PS C:\Projects\gotest> $Env:GOFLAGS="-ldflags=-X=main.Version=0.0.1 -ldflags=-X=main.Version2=0.1"
PS C:\Projects\gotest> go build                                                                   
PS C:\Projects\gotest> ./m.exe
Version1: dev, Version2: 0.1

So there is no way to pass multiple ldflags without proper quoting.

This test at least ensures that we are using this incorrect approach when passing ldflags: https://github.com/fyne-io/fyne-cross/blob/a730062af07dd5b3f45c74a465921b3b1e607fa6/internal/command/context_test.go#L92

andydotxyz commented 4 months ago

So there is no way to pass multiple ldflags without proper quoting.

Yes - but is that quoting not what was added in 1.5.0?

williambrode commented 4 months ago

I think you might be thinking of the quoting that is put around the value of an environment variable when it has an =. That exists, but the ldflags are still split before that point if there are spaces - which is totally wrong.

I went on to debug further, as my workaround of setting GOFLAGS directly still didn't work as I expected. I believe its related to this code in fyne build which pulls out the -ldflags from GOFLAGS, and tries to remove them from GOFLAGS: https://github.com/fyne-io/tools/blob/1ab79bc54deab687969154108a079030a4c964a1/cmd/fyne/internal/commands/build.go#L407. Only problem is, the calling code has already read the environment map before that, so we still have -ldflags in GOFLAGS. Not sure why that still wouldn't work though 🤷 but its an ineffective assignment by the looks of it.

williambrode commented 4 months ago

I was able to track down my issue. I've explained it in the PR here:

https://github.com/fyne-io/tools/pull/17

This was why the ldflags were ignored - they are converted to GOFLAGS in fyne cross, which are ignored by fyne build.

andydotxyz commented 4 months ago

@Bluebugs can you comment on this? I'm a bit lost.