Read from FyneApp.toml when developing with go tools #4715

andydotxyz · 2024-03-11T21:14:06Z

Fixes the go run and go build finding metadata during development.

Checklist:

Tests included. <- I didn't want to add Go compile (times) to the test process...
Lint and formatter run with no errors.
Tests all pass.

This means we can just 'go run .' and get icons etc. Fixes fyne-io#4688

Jacalz · 2024-03-11T22:01:34Z

I’m not entirely sure about this. Now every app will try to load FyneApp.toml on startup from the local directory. That doesn’t go well with the fact that this is a very specific developer problem mostly on Windows. 99% of all non development installs will not use this.

I think we either need a better solution or make sure that this functionality is turned off when building a release binary. I prefer the former though as I can’t build Flatpak apps with -tags release.

Jacalz · 2024-03-11T22:03:18Z

Maybe the better option just simply is to allow the developer to turn off the windowsgui linker flag?

andydotxyz · 2024-03-12T11:23:32Z

Maybe the better option just simply is to allow the developer to turn off the windowsgui linker flag?

I don't understand this comment? The change enables usage of "go run" lifecycle and using the "run" button inside IDEs.
It isn't only about fixing the issue where the windows console is not connected.

andydotxyz · 2024-03-12T11:25:10Z

Now every app will try to load FyneApp.toml on startup from the local directory.

Unless I coded it wrong I don't think this is the case. It will only look for local metadata if none was packaged. Which means for any packaged app, or released bundle, this code will never execute.

That doesn’t go well with the fact that this is a very specific developer problem mostly on Windows. 99% of all non development installs will not use this.

Erm... this is something that is useful to anybody wanting to use standard Go tools for their development cycle instead of packaging then running if their app uses metadata...

Jacalz · 2024-03-12T11:58:30Z

I’m sorry if my comments didn’t summarise clearly what I meant. The use case in the issue describes Windows and not wanting -H windowsgui as a linker flag and hence wanting to use the regular go commands. We added “fyne build” to have a way for embedding metadata for builds in the first place. Why work around that and add runtime lookup when not using it? I’d be more in favour of adding a “fyne run” command and potentially an option to turn off -H windowsgui for Windows developers that care about it.

If a user wants to have metadata during development, then switching to “fyne build” from “go build” is not such a bug change. We did design the build command to basically a drop in replacement. If that’s not the case for some use cases we might want to look into that.

Jacalz · 2024-03-12T12:03:22Z

Unless I coded it wrong I don't think this is the case. It will only look for local metadata if none was packaged. Which means for any packaged app, or released bundle, this code will never execute.

Yes, I expressed myself incorrectly there. What I mean is that there are cases where we have production builds that just can't use the fyne command for building. This basically applies to Flatpak and many Linux package build systems where there is sandboxing and no network access to install the command. I.e. all of those apps will now try to load a FyneApp.toml from the local directory which I'm not a huge fan of.

andydotxyz · 2024-03-12T12:06:29Z

This basically applies to Flatpak and many Linux package build systems where there is sandboxing and no network access to install the command. I.e. all of those apps will now try to load a FyneApp.toml from the local directory which I'm not a huge fan of.

I think this is a different problem and not one I had fully comprehended - they will not support metadata at all in the current state, or after the PR lands (as the file will be missing at runtime)

andydotxyz · 2024-03-12T12:08:21Z

I’d be more in favour of adding a “fyne run” command and potentially an option to turn off -H windowsgui for Windows developers that care about it.

This just seems like complexity to me. The Go tools create simple binaries and the fyne tool is for bundling graphical apps. This seems like a strong divide that we should stick to.

Jacalz · 2024-03-12T12:13:20Z

The Go tools create simple binaries and the fyne tool is for bundling graphical apps. This seems like a strong divide that we should stick to.

I don't quite agree with that given that we added the "fyne build" command for the very reason of building regular binaries in the same sense as "go build" without any bundling but with customisations to make the binaries more adapted to guis.

andydotxyz · 2024-03-12T12:30:33Z

The Go tools create simple binaries and the fyne tool is for bundling graphical apps. This seems like a strong divide that we should stick to.

I don't quite agree with that given that we added the "fyne build" command for the very reason of building regular binaries in the same sense as "go build" without any bundling but with customisations to make the binaries more adapted to guis.

Maybe I used the wrong words, but the sentiments are basically the same - go tools for CLI and fyne tools for GUI apps. If you add an option to create a GUI app without it being a GUI app then we will have to explain complex technical issues specific to OS that (if we have done our job right) people should never have to worry about.

app/meta_development.go

Jacalz

I added some comments inline. Also, should we try to not compile in the runtime metadata lookup if we know that the metadata was added? I.e. should we avoid compiling it in if we have release as build mode?

One thought (not necessarily something you need to fix): This can theoretically lead to strange behaviour if a binary was built without metadata in the past and runs in a directory that has a FyneApp.toml file. Might not be a meaningful problem but I feel it is worth noting.

app/app_openurl_web.go

Jacalz · 2024-03-12T15:24:44Z

app/meta_development.go

@@ -0,0 +1,40 @@
+//go:build !no_metadata


Instead of more complexity (more files) in the app/ folder, can you not use the new internal/build package and add a build.NoMetadata constant?

You can then just to an if build.NoMetadata before calling the function. Mates things a bit clearer, I think :)

Instead of more complexity (more files) in the app/ folder, can you not use the new internal/build package and add a build.NoMetadata constant?

You can then just to an if build.NoMetadata before calling the function. Mates things a bit clearer, I think :)

Won't this still be another new file? by placing it in the build folder it is disconnected from the metadata handling code.

I don't have a strong opinion one way or another - but isn't it a case of introducing a new constant and yet we have the same number of new files?

You are right that it won't result in less files but it would avoid adding more files to the already crowded app/ folder. The files in the other package are easier to grasp what they do. It is also consistent with how we check other build tags using that internal/build package.

OK done.

Just to be clear this change means:

1 extra file

A package public constant checked in 1 place

the toml package cannot be ignored during compile

The first two are correct but will have less meaning if we use them in more places in the future. You can move the single function into another file also if you want to as well.

The third one, about toml not being ignored should not be correct. As the NoMetadata and Type values are constant, the compiler can dead-code eliminate the branches where they are used.

The third one, about toml not being ignored should not be correct. As the NoMetadata and Type values are constant, the compiler can dead-code eliminate the branches where they are used.

I guess this is getting a little into semantics but I think we are both right. The package cannot be ignored during compile - however it may be possible that the compiler later optimises it out as you say.

Yup. It will likely be downloaded but it should not be part of the binary :)

Jacalz · 2024-03-12T15:31:28Z

app/meta_development.go

+)
+
+func checkLocalMetadata() {
+	file, _ := filepath.Abs("FyneApp.toml")


You might want to check the error here, I think

This went away

Jacalz · 2024-03-19T16:02:37Z

Sorry, I realised something. It is worth noting that this does bring in the toml dependency into every binary. You ought to check binary sizes and add that checkbox back to the PR template.

Jacalz · 2024-03-19T16:40:38Z

Thinking a bit more about this, I'm afraid that think this kind of brings more downsides for applications than upsides in its current implementation. Bundling the toml parsing into every app, apps without metadata will try to load metadata from PWD (unless it uses that tag to avoid it) and an app without metadata potentially loading the wrong metadata if launched from another directory seems bad in my opinion.

I'd instead propose adding a "runtime_metadata" tag to explicitly enable this for those that want it. If we introduce a "fyne run" command, it can use that tag automatically.

andydotxyz · 2024-03-19T18:30:04Z

I take your point - but adding a flag to do this essentially removes the point of it.
The idea was that go tools would just work. Requiring an additional flag is a different, but still additional, requirement for metadata to function.

If you do add that flag then the same problem you report with taking the wrong data still occurs for any apps built with that turned on.

Jacalz · 2024-03-19T19:45:45Z

I take your point - but adding a flag to do this essentially removes the point of it.
The idea was that go tools would just work. Requiring an additional flag is a different, but still additional, requirement for metadata to function.

We introduced the "fyne build" command basically to support metadata because "go build" couldn't. I agree that we for the most part shouldn't force people to use our cmd commands but I do see a tag for runtime metadata lookup as the best option given the tradeoffs mentioned above.

If you do add that flag then the same problem you report with taking the wrong data still occurs for any apps built with that turned on.

Yes but there is a very important distinction there. The potentially problematic behaviour is no longer by default and won't happen without you knowing.

dweymouth · 2024-03-19T19:53:25Z

I agree with @Jacalz - expecting go build and go run to "just work" with something outside the Go ecosystem (FyneApp.toml) is IMO a bit too much of an ask. But fyne build can be made to "just work" as we can inject the metadata at build time. I do think the better option is to just introduce fyne run

…xecuting in, if we can.

andydotxyz · 2024-03-20T15:13:56Z

Bundling the toml parsing into every app, apps without metadata will try to load metadata from PWD (unless it uses that tag to avoid it) and an app without metadata potentially loading the wrong metadata if launched from another directory seems bad in my opinion.

This has been addressed with the latest commit - loading only from the source directory (or PWD if it's a temporary go build i.e. "go run").

andydotxyz · 2024-03-20T15:20:26Z

As a general comment I don't like the idea that "just working" is something we should /not/ strive for...
There has been so much work put into making the idiosyncrasies of platforms and tools go away. For example:

macOS apps can only display notifications if they have been installed as a full app bundle - so we add a fallback for when running as go run or other pre-release package format.
building for web browser required specific versions of Go runtime to be installed so we stripped out GopherJS so that it would work with whatever versions the developer is running.

I would hope that everything we do is to make either the user or the developers life easier, and to take the path of least surprise.

dweymouth · 2024-03-20T15:34:33Z

Alright, last comment on trying to bridge the philosophical gap here between @andydotxyz and @Jacalz + myself. What if we use -tags release to disable the runtime meta lookup instead of introducing a new no_metadata tag? That way this behavior - which is intended as a develop-time convenience - won't get baked in release apps by default, rather than requiring developers to know about the new tag to avoid the run-time baggage.

Jacalz · 2024-03-20T15:57:09Z

What if we use -tags release to disable the runtime meta lookup instead of introducing a new no_metadata tag?

I already suggested disabling it for the release tag further up in the thread (during my code review). However, we can't rely on that tag being set for Flatpak and Linux builds in general.

Even if we add a no_metadata tag everyone creating Linux packages have the same issue of having to know about the tag, just like anyone using a runtime_metadata tag would have to know about it during development. Both solutions provide problems in one way or another.

Jacalz · 2024-03-20T15:59:45Z

I kind of think it is easier to put the "you need to look up this compile tag to improve things" on the developer of the Fyne app rather than the package maintainer.

andydotxyz · 2024-03-20T21:31:37Z

I'm sorry but I don't agree with this. The go tools and source build are used a lot more by developers and consumers of GitHub code than packagers.
Developer tools are primarily for development and not packaging.

With the current exception of flatpak app packaging is done with "fyne" tool.

Jacalz · 2024-03-20T23:10:24Z

With the current exception of flatpak app packaging is done with "fyne" tool.

Not Flatpak only. I'd say that it is more like all of the Linux packages in repositories. I.e. Rymdport I'm AUR, Solus, Nix and so on. That's the majority of Linux packages there. Might even apply to BSD* if their build systems behave in a similar way.

andydotxyz · 2024-03-21T09:35:42Z

Not Flatpak only. I'd say that it is more like all of the Linux packages in repositories. I.e. Rymdport I'm AUR, Solus, Nix and so on. That's the majority of Linux packages there. Might even apply to BSD* if their build systems behave in a similar way.

I think we are losing sight of the purpose of the tools we have created. The fyne command is to help with packaging and other aspects of GUI app creation that are required along the path to getting your app out there. It is not a development tool. When I am preparing linux builds I use the fyne commands and it works great. The exception we are dealing with is that the flatpak build system apparently won't allow the use of this 3rd-party tool so we have to work around it. It feels like we are focusing on the exceptions rather than the design principles in this discussion. During development this should all just work.

andydotxyz · 2024-03-21T09:40:19Z

I realise now that the no_metadata flag really would solve my issues as long as we make sure to have -tags release turn it off as well (so release binaries don't compile in the whole TOML parser).

Good idea - done.

This might make things more sensible to reason about. So, I have some code to make sure that I'm not bundling the icon twice.

There should be no code required in applications to make sure that metadata is all bundled correctly. With these changes it will work in all cases except when you are manually turning metadata off. If that is a desire of your app then the same build flags can be used to insert your own version of metadata instead. No overlap and no ambiguity.

andydotxyz · 2024-03-21T09:42:27Z

If we expose the metadata parser as a metadata.Parse([]byte) fyne.Metadata function, it would allow the user to do this if they want to

This is returning to the old problem of "why is my icon not working" bugs where the code was missing, so we added automatic metadata handling so that it just worked. Reverting to the old method is going to introduce new unexpected corner cases and require that people a) read the documentation and b) insert more code into their app to get it working for the default tooling. Both are things I would like to avoid, especially as we saw this exact thing play out with app icons before it.

Jacalz · 2024-03-21T10:00:57Z

Would you mind addressing the review comments that I have inline from the last review? I think they are still valid.

Jacalz · 2024-03-21T10:08:21Z

This is returning to the old problem of "why is my icon not working" bugs where the code was missing, so we added automatic metadata handling so that it just worked.

It was just a thought experiment around using more go:embed to spark some ideas :)

Jacalz · 2024-03-21T10:10:50Z

The exception we are dealing with is that the flatpak build system apparently won't allow the use of this 3rd-party tool so we have to work around it.

All I was trying to say is that it isn't an exception given that it basically applies to all Linux package systems because we can't rely on the cmd/fyne version being the same as what we are statically linking in out apps (if it even exists as a package). Anyway, I think this the issues that I brought up have been addressed with the latest changes.

andydotxyz · 2024-03-21T11:38:29Z

All I was trying to say is that it isn't an exception given that it basically applies to all Linux package systems because we can't rely on the cmd/fyne version being the same as what we are statically linking

I think that "packager does not have the latest packaging tool installed" can be considered a corner case. It's surely in the same camp as "packager's Go version is not new enough for this app"?

andydotxyz · 2024-03-21T12:59:48Z

Would you mind addressing the review comments that I have inline from the last review? I think they are still valid.

Done. As far as I can see it was only a missed defer that still applied.

Jacalz · 2024-03-21T14:42:03Z

Done. As far as I can see it was only a missed defer that still applied.

I think this comment still is valid:
#4715 (comment)

Jacalz · 2024-03-21T14:47:01Z

I think that "packager does not have the latest packaging tool installed" can be considered a corner case. It's surely in the same camp as "packager's Go version is not new enough for this app"?

I wouldn't directly put it in the same camp. One makes it impossible to compile and the other would just not support metadata or some other missing functionality.

EDIT: Yes, in the case where it might break stuff.

It is also worth noting that on an LTS version of Linux, the fyne command will likely never be as new as the Fyne version of the app (if we assume they stick to a minor release for the whole life cycle). A rolling release would potentially be less of an issue but there it might instead be too new if you haven't updated the app in a while.

andydotxyz · 2024-03-21T15:09:49Z

It is also worth noting that on an LTS version of Linux, the fyne command will likely never be as new as the Fyne version of the app (if we assume they stick to a minor release for the whole life cycle).

Indeed that possibly poses a problem - as it always has done. But of course we have Fyne-cross if people don't have the right versions of tooling for the packaging process.

andydotxyz · 2024-03-21T15:10:48Z

Done. As far as I can see it was only a missed defer that still applied.

I think this comment still is valid: #4715 (comment)

Apologies, for some reason GitHub had completely hidden that.

andydotxyz · 2024-03-22T15:23:39Z

OK, I think this is now a solution that

a) "just works" for most go based development flow
b) will only use the FyneApp.toml next to the executable, unless invoked during "go run" in which case it uses PWD (as the bin is actually in /tmp)
c) can be switched off manually and will be disabled for release builds

dweymouth · 2024-03-22T15:34:15Z

I would just check for binary size increases with a release build, and if it doesn't go up, or goes up minimally (a few %?), I'm fine with this now

andydotxyz · 2024-03-22T16:11:55Z

Looks like a release (or no_metadata) build is 18KB increase, that's 0.06%.

Jacalz · 2024-03-22T17:05:47Z

Sounds very sensible. Bigger difference for a regular development build?

andydotxyz · 2024-03-22T20:57:07Z

Sounds very sensible. Bigger difference for a regular development build?

yeah it was around 2/3 of 1%

andydotxyz · 2024-03-22T20:58:35Z

I think the changes requested are all sorted @Jacalz

Jacalz · 2024-03-22T23:32:59Z

I think so too after a quick glance. Will review once I'm back at my computer on Monday

Jacalz

Just one minor comment that has some strange wording. Approved in all other ways :)

Jacalz · 2024-04-03T21:34:50Z

app/meta_development.go

+		return work
+	}
+
+	// we were called with after "go build" completed


I think there is a mistake in this comment. The wording does not quite make sense?

andydotxyz added 2 commits March 11, 2024 21:07

Support reading metadata file in development mode

9b01be2

This means we can just 'go run .' and get icons etc. Fixes fyne-io#4688

Fix naming warnings for reciever

2e7b623

andydotxyz mentioned this pull request Mar 12, 2024

Please add a way to embed the FyneApp.toml metadata into the go build. #4688

Closed

2 tasks

andydotxyz mentioned this pull request Mar 12, 2024

Add fyne run command to cli fyne-io/tools#12

Open

2 tasks

Allow metadata lookup to be turned off through a build tag

4577b8b

dweymouth reviewed Mar 12, 2024

View reviewed changes

app/meta_development.go Show resolved Hide resolved

Jacalz requested changes Mar 12, 2024

View reviewed changes

Don't look up any old metadata file - only use the directory we are e…

793a25d

…xecuting in, if we can.

Disable runtime metadata lookup for release builds

41b2cad

Missed defer Close

7f4ebec

Move the no_metadata flag to build package

9ec4ac6

dweymouth self-requested a review March 22, 2024 17:45

dweymouth previously approved these changes Mar 22, 2024

View reviewed changes

Jacalz requested changes Apr 3, 2024

View reviewed changes

Fix comment

0381460

andydotxyz dismissed dweymouth’s stale review via 0381460 April 4, 2024 18:05

Jacalz approved these changes Apr 4, 2024

View reviewed changes

andydotxyz merged commit 7e2bb66 into fyne-io:develop Apr 4, 2024
11 checks passed

andydotxyz deleted the fix/4688 branch April 4, 2024 20:37

Read from FyneApp.toml when developing with go tools #4715

Read from FyneApp.toml when developing with go tools #4715

Conversation

andydotxyz commented Mar 11, 2024 • edited Loading

Checklist:

Jacalz commented Mar 11, 2024 • edited Loading

Jacalz commented Mar 11, 2024

andydotxyz commented Mar 12, 2024

andydotxyz commented Mar 12, 2024

Jacalz commented Mar 12, 2024

Jacalz commented Mar 12, 2024 • edited Loading

andydotxyz commented Mar 12, 2024 • edited by Jacalz Loading

andydotxyz commented Mar 12, 2024

Jacalz commented Mar 12, 2024

andydotxyz commented Mar 12, 2024

Jacalz left a comment

Choose a reason for hiding this comment

Jacalz Mar 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jacalz Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jacalz commented Mar 19, 2024

Jacalz commented Mar 19, 2024 • edited Loading

andydotxyz commented Mar 19, 2024

Jacalz commented Mar 19, 2024 • edited Loading

dweymouth commented Mar 19, 2024

andydotxyz commented Mar 20, 2024

andydotxyz commented Mar 20, 2024

dweymouth commented Mar 20, 2024

Jacalz commented Mar 20, 2024

Jacalz commented Mar 20, 2024

andydotxyz commented Mar 20, 2024

Jacalz commented Mar 20, 2024

andydotxyz commented Mar 21, 2024

andydotxyz commented Mar 21, 2024

andydotxyz commented Mar 21, 2024

Jacalz commented Mar 21, 2024

Jacalz commented Mar 21, 2024

Jacalz commented Mar 21, 2024

andydotxyz commented Mar 21, 2024

andydotxyz commented Mar 21, 2024

Jacalz commented Mar 21, 2024

Jacalz commented Mar 21, 2024 • edited Loading

andydotxyz commented Mar 21, 2024

andydotxyz commented Mar 21, 2024

andydotxyz commented Mar 22, 2024

dweymouth commented Mar 22, 2024

andydotxyz commented Mar 22, 2024

Jacalz commented Mar 22, 2024

andydotxyz commented Mar 22, 2024

andydotxyz commented Mar 22, 2024

Jacalz commented Mar 22, 2024

Jacalz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andydotxyz commented Mar 11, 2024 •

edited

Loading

Jacalz commented Mar 11, 2024 •

edited

Loading

Jacalz commented Mar 12, 2024 •

edited

Loading

andydotxyz commented Mar 12, 2024 •

edited by Jacalz

Loading

Jacalz Mar 12, 2024 •

edited

Loading

Jacalz Mar 22, 2024 •

edited

Loading

Jacalz commented Mar 19, 2024 •

edited

Loading

Jacalz commented Mar 19, 2024 •

edited

Loading

Jacalz commented Mar 21, 2024 •

edited

Loading