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

Bugs/support go1.20 darwin #194

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Conversation

Bluebugs
Copy link
Contributor

@Bluebugs Bluebugs commented May 6, 2023

Description:

So apparently with go 1.20, we need to add -lresolv manually for some reason on darwin when any dependencies use the net package :-(

See golang/go#58416 and golang/go#58159

I know we could rely on the user having to set it manually, but I think that having it by default will help fyne-cross user and reduce the potential bug report. We could also detect if a project use the net package and add it only if needed, but I don't think it is a big deal to always add it.

@Bluebugs Bluebugs requested a review from lucor May 6, 2023 04:16
@andydotxyz
Copy link
Member

Are you sure about this? The linked bugs both seem to be referencing Xcode / gomobile - which is iOS not Darwin in our build tooling.
I am pretty sure this is not required for all Darwin as I have been using go 1.20 for months with no issue.

@Bluebugs
Copy link
Contributor Author

Bluebugs commented May 6, 2023

Are you sure about this? The linked bugs both seem to be referencing Xcode / gomobile - which is iOS not Darwin in our build tooling.
I am pretty sure this is not required for all Darwin as I have been using go 1.20 for months with no issue.

There is two bugs report. One for ios and one for Darwin. As said, this only impact your application is you use the net package.

@@ -66,9 +67,8 @@ func (cmd *darwin) Parse(args []string) error {
// Add flags to use only on darwin host
if runtime.GOOS == darwinOS {
flagSet.BoolVar(&cmd.localBuild, "local", true, "If set uses the fyne CLI tool installed on the host in place of the docker images")
} else {
Copy link
Member

Choose a reason for hiding this comment

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

This will enable the flag also for build on macOS host. I think it is ok, but probably we need to adjust also the logic here

if !cmd.localBuild {

@Jacalz
Copy link
Member

Jacalz commented May 11, 2023

I believe that my Geoffrey build 295 of Rymdport just failed due to this error. I don't see the logs myself but you can probably double-check them to see if that is the case.

EDIT: Apparently another issue according to Cedric. Geoffrey is already using this PR.

@andydotxyz
Copy link
Member

andydotxyz commented May 20, 2023

To take another approach, this code was compiled on an Apple M2 without any special flags:

package main

import (
	"net"

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

func main() {
	a := app.New()
	w := a.NewWindow("Hello")

	is, _ := net.Interfaces()
	w.SetContent(widget.NewLabel(is[0].Name))

	w.ShowAndRun()
}

(just FYI the mobile packager fyne package -os ios -profile "Wildcard Dev" works as expected on M2)

@andydotxyz
Copy link
Member

I have also tested the same source code on a build for Darwin using Fyne-cross v1.3.0 and v1.4.0, both succeed without this patch.
I guess what I am struggling with is how to replicate the problem to test this fix with.

@Bluebugs
Copy link
Contributor Author

The issue happen as described in both go ticket when you use anything in the net package as that requires dns resolution now provided by that library.

@Jacalz
Copy link
Member

Jacalz commented May 26, 2023

But the code mentioned above does use the net package?

@Bluebugs
Copy link
Contributor Author

But the code mentioned above does use the net package?

That code is not going to require any dns resolution which is when you will see the problem.

@andydotxyz
Copy link
Member

OK, the following compiles and runs with go 1.20.3 without any parameters...

package main

import (
	"log"
	"net"

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

func main() {
	a := app.New()
	w := a.NewWindow("Hello")

	addr, err := net.LookupIP("google.com")
	if err != nil {
		log.Println("ERR", err)
	}
	w.SetContent(widget.NewLabel(addr[0].String()))

	w.ShowAndRun()
}

@Bluebugs
Copy link
Contributor Author

Bluebugs commented Jun 2, 2023

As the original bug report point out, it is hard to have a small reproducible bug case and the one who opened the bug report are long time contributor to go. You might have luck with bigger application doing something. You can try mqttweather, maybe nomad or my application gotik. You could also just try to compile tailscale as that was the initial report on those bug.

@andydotxyz
Copy link
Member

I don't know what to do with this - how can it progress without the ability to replicate the problem or confirm if it applies in our instance?

Is it possible for you to share more about how it was discovered or which projects do not work without the change?

@Bluebugs
Copy link
Contributor Author

Bluebugs commented Jun 3, 2023

As I said try with big complex applications that use the network. Considering the bug report upstream come from tailscale developer. Just add tailscale to an application should lead to reproduce the problem, but mqttweather, rhymdport and nomad are likely also going to reproduce the problem.

@Bluebugs
Copy link
Contributor Author

The release note for go 1.20 have been updated to include the need to add -lresolv when using cgo on mac with the net package: https://tip.golang.org/doc/go1.20 . This patch now follow official Go documentation and should not be blocked any longer.

@Jacalz
Copy link
Member

Jacalz commented Nov 19, 2023

But we are not building with -buildmode=c-archive? Rymdport uses the net package and I haven't seen anyone report any problems.

@Bluebugs
Copy link
Contributor Author

Bluebugs commented Nov 19, 2023

My understanding is that option is added of you use any dependency that are static. That might be the case with fyne and glfw when using net with some dns resolution as that's what mqttweather does. As use of tailscale trigger it anyway, I am guessing it's because of some option they are setting somewhere to enable their library to be use by c code.

@Bluebugs
Copy link
Contributor Author

As for rhymdport did you try building it on Mac with latest go lately?

@Jacalz
Copy link
Member

Jacalz commented Nov 20, 2023

I have been building it using Geoffrey

@Jacalz
Copy link
Member

Jacalz commented Nov 20, 2023

Which I realize probably is the reason that I haven't noticed it to be fair...

@andydotxyz
Copy link
Member

I am trying to get back to things and it seems that this may have been updated on the Go docs, vis:

A consequence is that, on macOS, if Go code that uses the net package is built with -buildmode=c-archive, linking the resulting archive into a C program requires passing -lresolv when linking the C code.

This seems more clear. As far as I am aware we are not building C archives and then linking them into a Fyne application - so we should be OK to not make this change shouldn't we? This does explain why all my local builds kept working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants