Refactor and reuse code #101

Bluebugs · 2022-04-06T17:40:10Z

Description:

This is "just" a refactor. As I was working on #98, I realize it would be simpler if there was a bit more potential for reusing code around. I am not sure that all of those change are really idiomatic go and feel like doing a PR that "just" move code around and remove duplicated step will make the next PR easier to move forward.

Checklist:

Tests included.
Lint and formatter run with no errors.
Tests all pass.

Where applicable:

There should be no functional change. I have only the ability to test with podman at the moment and will be glad if anyone tried with docker.

lucor · 2022-04-07T18:40:17Z

I had a cursory look at PR and I would say that the refactor make sense to me but some thoughts come in mind... :-)
Looking at the amount of code changed I was thinking if it could be the right time to move fyne-cross as command of the fyne CLI app, something like fyne cross.
Having it as part of the fyne CLI will bring the following pros:

remove the direct dependency from the Docker image. This will reduce the number of releases and possible issues due to versions not aligned (see fyne-cross fails when trying to create TAR file. #85)
all the fyne CLI APIs can be invoked directly, no need to use the exec.Cmd (thinking about the build and package steps here)
uniform the CLI APIs and flags (see parameters and their documentation are inconsistent #46)
the support to FyneApp.toml and build metadata would not require code duplication (see Add support for FyneApp.toml #87 (comment))

Not strictly related but something could be helpful is to move the Docker image code into a dedicated repository (i.e. fyne-cross-image). Right now it they are different projects with a proper life-cycle that shares the same repo.

Thoughts?

Bluebugs · 2022-04-07T18:59:37Z

I personally like the idea of moving fyne-cross to become fyne cross and get synchronized release. It will have to be discussed with the rest of the community. Maybe we should create a set of issues toward that goal in fyne-cross and aim for inclusion of fyne-cross in fyne for 2.3.

Jacalz

I suspect that @lucor will have more (if anything) to say about the code. It all looks sensible to me but I don't quite have the same knowledge over the codebase. All my local builds of rymdport also work just as before, hence approved :)

andydotxyz · 2022-04-11T15:15:46Z

could be the right time to move fyne-cross as command of the fyne CLI app, something like fyne cross.

This is a great thing to work towards, however I don't think I would have it as a separate command (fyne cross) but more likely an extension of the whole command where we improve the cross-compile system and allow the fyne-cross path to be run if/when appropriate.

lucor · 2022-04-11T19:17:35Z

Good point, that makes sense to me. We already do a similar thing when building from a macOS with a darwin target

lucor

Had a little in depth look at the code and the most of the feedback are related about the naming like: ContainerImage, AllContainerImage, AllContainerRunner.

Anyway all changes are internal and it works locally using docker as before, so feel free to merge if we need it for #108. I'd not block this with a discussion about naming :) Probably they will be lost during the porting to fyne CLI and in any case we can always address them in to a later stage.

internal/command/command.go

lucor · 2022-04-14T07:10:32Z

internal/command/container.go

+)
+
+type ContainerRunner interface {
+	NewImageContainer(arch Architecture, OS string, image string) ContainerImage


Not sure about having a constructor defined as part of an interface

Agreed, that isn't the Go pattern, It is not clear what ContainerRunner is describing.

Thinking about it, ContainerRunner is basically the engine definition (podman, docker, kubernetes).

Ah, so Engine could work, remove the duplication that's tripping me up?
Could use Create instead of New so it's not looking like a constructor?
Also I realise here there is ImageContainer vs ContainerImage - which is desired?

I think ContainerImage.

lucor · 2022-04-14T07:11:29Z

internal/command/container.go

+	Debug(v ...interface{})
+	GetDebug() bool


What about moving these methods into a dedicated interface, like Debugger ?

internal/command/container.go

lucor · 2022-04-14T07:18:05Z

internal/command/container.go

+	GetArchitecture() Architecture
+	GetOS() string
+	GetID() string
+	GetTarget() string
+	GetEnv(string) (string, bool)
+	SetEnv(string, string)
+	SetMount(string, string)
+	AppendTag(string)


Do we really need to add these methods as part of an interface ? Should be they removed in favor of a contract with the AllContainerImage struct ?

That sounds doable. I was thinking I would have to change the behavior of some of them, but actually not at all.

Remember that in Go it is idiomatic to remove Get prefix for simple getter calls.

Are the Os/Arch part of the API at all? a ContainerImage is a generic construct that is created by the ContainerRunner calling some constructor passing in state, so this feels a little circular.

After trying to remove them, it actually is more useful than expected as code use a ContainerImage to build figure out what command to execute. A ContainerImage is a representation of an Engine/OS/Architecture capability. I think they are for the moment useful where they are.

internal/command/container.go

lucor · 2022-04-14T18:25:57Z

@Bluebugs it looks like the android command is not available:

The commands are:

	darwin-image  Build the docker image for darwin
	darwin        Build and package a fyne application for the darwin OS
	linux         Build and package a fyne application for the linux OS
	windows       Build and package a fyne application for the windows OS
	              
	ios           Build and package a fyne application for the iOS OS
	freebsd       Build and package a fyne application for the freebsd OS
	version       Print the fyne-cross version information

Use "fyne-cross <command> -help" for more information about a command.```

Bluebugs · 2022-04-14T18:52:35Z

@Bluebugs it looks like the android command is not available:

Indeed, good catch. I missed that part. Will fix it shortly.

internal/command/container.go

andydotxyz · 2022-04-15T08:02:02Z

I have added some thoughts around naming. However I was not able to add suggestions in most places as it is not clear of the intent from model presented. Can you please summarise what each of the new interfaces/structs are supposed to describe in one comment so we can understand the bigger picture @Bluebugs? It is a bit confusing that runners contain images bit images have run methods.

Bluebugs · 2022-04-18T19:18:48Z

Sounds like a lot of good suggestion. I can have a pass at it tomorrow and we can iterate on the change after that.

Bluebugs · 2022-05-11T21:41:29Z

Small attempt at a diagram, not sure it is super usable:

classDiagram
   Command<|--Linux
   Command<|--Windows
   CrossBuilder<|--Linux
   CrossBuilder<|--Windows

   class Command{
     <<Interface>>
     +Run() error
   }
   class CrossBuilder{
     <<Interface>>
     +Build(ContainerImage)
     -setupContainerImage(flags, args) error
   }

   Command-->ContainerImage:Run() Use
   CrossBuilder-->ContainerImage:Build() Use
   CrossBuilder-->ContainerEngine:setupContainerImage() Use

   ContainerEngine<|--LocalContainerEngine
   ContainerImage<|--LocalContainerImage
   ContainerEngine<|--KubernetesContainerEngine
   ContainerImage<|--KubernetesContainerImage

   class ContainerEngine{
     <<Interface>>
     +createContainerImage(arch Architecture, OS string, image string) containerImage
   }

   class ContainerImage{
     <<Interface>>
     +Run(vol volume.Volume, opts options, cmdArgs []string) error
   }

   ContainerEngine-->ContainerImage:create

andydotxyz · 2022-05-14T12:38:37Z

That is really helpful thanks - thought it is missing the PlatformBuilder which was part of my confusion from earlier.
Apart from that the diagram makes sense - if it included the Fields that were of the object types as well we might see the circular relationships emerge.

Bluebugs · 2022-05-20T00:16:13Z

Adding platformBuilder which is being used by CrossBuilder to provide per platform specific build step (The OS specific structure also rely on CrossBuilder to provide actually a portion of the Command interface that was duplicated across all OS before). I think the arrow for most field that have structure or interface are actually visible here.

classDiagram
   Command<|--Linux
   Command<|--Windows
   platformBuilder<|--Linux
   platformBuilder<|--Windows

   class Command{
     <<Interface>>
     +Run() error
   }
   class CrossBuilder{
     <<Interface>>
     +platformBuilder
     +Run()
     -setupContainerImage(flags, args) error
   }
   class platformBuilder{
     <<Interface>>
     +Build(ContainerImage)
   }

   CrossBuilder-->ContainerImage
   CrossBuilder-->ContainerEngine:setupContainerImage() Use
   CrossBuilder-->platformBuilder:Build() Use
   platformBuilder-->ContainerImage
   Command<|--CrossBuilder

   ContainerEngine<|--LocalContainerEngine
   ContainerImage<|--LocalContainerImage
   ContainerEngine<|--KubernetesContainerEngine
   ContainerImage<|--KubernetesContainerImage

   class ContainerEngine{
     <<Interface>>
     +createContainerImage(arch Architecture, OS string, image string) containerImage
   }

   class ContainerImage{
     <<Interface>>
     +Run(vol volume.Volume, opts options, cmdArgs []string) error
   }

   ContainerEngine-->ContainerImage:create

andydotxyz · 2022-05-24T17:09:38Z

internal/command/container.go

@@ -59,6 +58,7 @@ type containerImage interface {
 	Tags() []string

 	Engine() containerEngine
+	Debugger() debugger


This looks more like moved rather than removed. In this new position all containerImages must be debuggable. Is this a desirable definition of an image?
Go loose typing indicates that anything could be debuggable, if it provides the facilities to be debugged, rather than stating which interfaces will have to support debugging...

Hum, I am not sure that I get what you want correctly. Do you mean, I should always cast all Debugger call on the Engine()? Something like: debug, ok := image.Engine().(Debugger) and then if the debugger is present do call the debugger function?

andydotxyz · 2022-05-24T17:27:23Z

Adding platformBuilder which is being used by CrossBuilder to provide per platform specific build step

Thanks for this, I think I am starting to understand.
Proposal: CrossBuilder is not an interface, it is our single struct type that is the core of this package. It would use a PlatformBuilderandContainerEngine` to function.

The OS specific structure also rely on CrossBuilder to provide actually a portion of the Command interface that was duplicated across all OS before

This feels messy - combining re-use with inheritance. Why not separate the requirement on CrossBuilder with some "command common code" type (I don't know what scope this has) that contains only this common code? Remember that Go supports multiple inheritance. This way the platform builders don't depend on the cross builder, which depends on them again.

I hope these ideas are helpful, It should take out some of the seemingly circular usages.

Bluebugs · 2022-05-24T21:45:26Z

Adding platformBuilder which is being used by CrossBuilder to provide per platform specific build step

Thanks for this, I think I am starting to understand. Proposal: CrossBuilder is not an interface, it is our single struct type that is the core of this package. It would use a PlatformBuilderandContainerEngine` to function.

It is pretty much this already, except it use a ContainerImage to function which is provided by a ContainerEngine.

The OS specific structure also rely on CrossBuilder to provide actually a portion of the Command interface that was duplicated across all OS before

This feels messy - combining re-use with inheritance. Why not separate the requirement on CrossBuilder with some "command common code" type (I don't know what scope this has) that contains only this common code? Remember that Go supports multiple inheritance. This way the platform builders don't depend on the cross builder, which depends on them again.

I kind of want to avoid touching much of the command part as it will disappear, I believe, once we migrate to cli.App.

andydotxyz · 2022-05-27T21:15:47Z

The OS specific structure also rely on CrossBuilder to provide actually a portion of the Command interface that was duplicated across all OS before

This feels messy - combining re-use with inheritance. Why not separate the requirement on CrossBuilder with some "command common code" type (I don't know what scope this has) that contains only this common code? Remember that Go supports multiple inheritance. This way the platform builders don't depend on the cross builder, which depends on them again.

I kind of want to avoid touching much of the command part as it will disappear, I believe, once we migrate to cli.App.

I see - but how does the current cli library require that each platform-specific builder rely on the cross builder?

Bluebugs · 2022-05-27T21:29:56Z

I see - but how does the current cli library require that each platform-specific builder rely on the cross builder?

The cross builder is the provider of Run() which really shouldn't be duplicated, but due to the Cli library requiring the existence of a Run per platform it has to be somewhere. The solution is to either duplicate code (bad) or reuse by depending on cross builder (better). Best will be when we do not need to link the crossBuilder.Run() with the Cli and can then just drop that from the dependencies.

andydotxyz · 2022-06-03T19:53:27Z

After discussing debugger on the call this is closer but I am still stuck on :

The solution is to either duplicate code (bad) or reuse by depending on cross builder (better).

isn’t there a third option where they don’t have to depend on one another but the common code is used by both?

Bluebugs · 2022-06-03T20:51:33Z

The solution is to either duplicate code (bad) or reuse by depending on cross builder (better).

isn’t there a third option where they don’t have to depend on one another but the common code is used by both?

I can not think of a different solution until we use cli.App that would be significantly different from any of the one above. I am convinced that using cli.App will solve this problem and that we won't need anymore that duplicated code, but switching to cli.App introduce massive change in the code base and this PR is already big enough that I do not want to do that here. Still I think that that would likely address your problem completely and be your third option.

…sor.

andydotxyz · 2022-06-06T21:43:25Z

Please don't force push after reviews started, it now needs a complete review again as we can't see the changes any more

Bluebugs · 2022-06-06T23:35:09Z

Please don't force push after reviews started, it now needs a complete review again as we can't see the changes any more

Sorry about that. I made a mistake in merging the wrong branch and I had to force push to undo that change.

andydotxyz · 2022-06-08T12:08:51Z

I had a look at the code and am not sure I agree that crossBuilder must exist because of the way the CLI works.
I see that it removes duplication of shared code, but I don't think it's the right way to do it because crossBuilder and platformBuilder create a tight circular dependency.

Looking at it there are 3 methods, two if which just return the name and description, which is different. The third, Run does all of the hard work. So why not:

return Name and Description back to the commands (platform builder entry points)
move the crossBuilder.Run to a helper method and remove crossBuilder completely
call the helper run method from each platform builder Run

This should be approx the same number of lines as you have, but with a cleaner control dependency graph.

Bluebugs · 2022-06-14T21:30:46Z

The build failure is unrelated as far as I know and due to a bug with v2.2

andydotxyz · 2022-06-15T07:22:06Z

The build failure is unrelated as far as I know and due to a bug with v2.2

Or maybe it's not a bug and this is now using Fyne v2.2.1 tools to compile a Fyne v2.1.4 app - we do ask that folk keep the versions in sync.

Probably we didn't see this coming but it is a side/effect of using the generated code to insert metadata. I guess we could improve the tooling to try and detect if SetMetadata is available.

andydotxyz · 2022-06-15T20:53:31Z

I have updated the calculator module and kicked the tests so we can see if that was the only error.

andydotxyz

I'm happy with how this turned out. I'd like to know that @lucor is too though :)

lucor

Agreed, additionally I won't make this blocker. The refactoring is related to internal packages and we can always iterate later, if needed.

andydotxyz · 2022-06-21T13:36:19Z

Perhaps we should make the current release, then land it to work on with more time?

Cedric BAIL added 4 commits April 6, 2022 11:34

Refactor code to split general context and per container operation.

19e4455

Mimick behavior in windows case with other OS end of image run case.

d75692d

Refactor end of each image run.

303b9a5

Move finalization of build and package transfer to ContainerImage.

e32eb44

Bluebugs requested review from lucor and Jacalz April 6, 2022 17:40

Cedric BAIL added 2 commits April 6, 2022 11:41

Address static check.

0cab06e

Make mount point iteration stable.

d48c6c3

Jacalz previously approved these changes Apr 9, 2022

View reviewed changes

lucor reviewed Apr 14, 2022

View reviewed changes

Forgotten Android initialization.

67d93c0

Bluebugs dismissed Jacalz’s stale review via 67d93c0 April 14, 2022 19:07

andydotxyz reviewed Apr 15, 2022

View reviewed changes

internal/command/container.go Outdated Show resolved Hide resolved

andydotxyz reviewed Apr 15, 2022

View reviewed changes

internal/command/container.go Outdated Show resolved Hide resolved

Cedric BAIL added 8 commits April 19, 2022 12:59

Rename ContainerRunner to ContainerEngine.

5305ed8

Rename AllContainerEngine to baseEngine.

e7783ef

Rename AllContainerImage to baseContainerImage

dc7046b

Enforce a cleaner Debugger interface.

d3e8c8a

Enforce ContainerImage naming.

6e59f33

Use Create inside Engine instead of constructor.

0d87566

Rename CrossBuilderCommand.RunEach to PlatformSpecific.Step.

afd522d

Remove Get prefix in ContainerImage.

395fc15

Unexport and rename everything that make sense.

cac0d93

Split debugger interface outside of the Engine interface.

ebdbab6

Bluebugs requested a review from andydotxyz May 24, 2022 14:28

andydotxyz reviewed May 24, 2022

View reviewed changes

Bluebugs requested review from andydotxyz June 3, 2022 16:01

Cedric BAIL added 2 commits June 3, 2022 17:03

Remove the debugger interface and use a global with a read only acces…

a879c8a

…sor.

Merge branch 'develop' into refactor-reuse-code

071d846

Bluebugs force-pushed the refactor-reuse-code branch from f626acd to 071d846 Compare June 3, 2022 23:14

Remove crossBuilder.

d2002fe

andydotxyz approved these changes Jun 20, 2022

View reviewed changes

lucor approved these changes Jun 21, 2022

View reviewed changes

Bluebugs merged commit 1c93082 into develop Jul 25, 2022

Bluebugs deleted the refactor-reuse-code branch July 25, 2022 16:18

Refactor and reuse code #101

Refactor and reuse code #101

Conversation

Bluebugs commented Apr 6, 2022 • edited Loading

Description:

Checklist:

Where applicable:

lucor commented Apr 7, 2022

Bluebugs commented Apr 7, 2022

Jacalz left a comment

Choose a reason for hiding this comment

andydotxyz commented Apr 11, 2022

lucor commented Apr 11, 2022

lucor left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucor commented Apr 14, 2022

Bluebugs commented Apr 14, 2022

andydotxyz commented Apr 15, 2022

Bluebugs commented Apr 18, 2022

Bluebugs commented May 11, 2022 • edited Loading

andydotxyz commented May 14, 2022

Bluebugs commented May 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andydotxyz commented May 24, 2022

Bluebugs commented May 24, 2022

andydotxyz commented May 27, 2022

Bluebugs commented May 27, 2022

andydotxyz commented Jun 3, 2022

Bluebugs commented Jun 3, 2022

andydotxyz commented Jun 6, 2022

Bluebugs commented Jun 6, 2022

andydotxyz commented Jun 8, 2022

Bluebugs commented Jun 14, 2022

andydotxyz commented Jun 15, 2022

andydotxyz commented Jun 15, 2022

andydotxyz left a comment

Choose a reason for hiding this comment

lucor left a comment

Choose a reason for hiding this comment

andydotxyz commented Jun 21, 2022

Bluebugs commented Apr 6, 2022 •

edited

Loading

Bluebugs commented May 11, 2022 •

edited

Loading