enable users to prepare the flutter engine within docker image without building first. #214

cosban · 2021-06-09T00:41:51Z

Fixes #598

For the details of why this pull request was made, see go-flutter-desktop/go-flutter#598

cosban · 2021-06-09T01:33:28Z

I'm getting a 404 error when I attempt to see why this failed and as such I'm not able to fix it.

When I run tests on my own machine, failures seem to occur because of line ending differences within sections of the project which this pull request does not touch.

provokateurin

 > [hover 29/30] RUN hover prepare-engine --all:
#47 1.058 hover: Downloading engine for platform linux-release at version a9d88a4d182bdae23e3a4989abfb7ea25954aad1...
Download completed in 1.75s
#47 4.724 hover: Downloading engine for platform windows-release at version a9d88a4d182bdae23e3a4989abfb7ea25954aad1...
Download completed in 1.42s
#47 8.362 hover: Downloading engine for platform darwin-release at version a9d88a4d182bdae23e3a4989abfb7ea25954aad1...
Download completed in 1.30s
#47 10.64 hover: install_name_tool failed: exec: "darling": executable file not found in $PATH
#47 10.64 hover:

I think you should remove the preparation of the darwin release engine, because it's not possible to cross compile darwin release from linux using docker. The build command already stops users from doing that.
So you should switch darwin release to debug, but I think windows and linux should also get the debug engine.

provokateurin · 2021-06-09T09:01:35Z

Dockerfile

@@ -121,4 +121,8 @@ RUN go install -v ./... 2>&1

 COPY docker/hover-safe.sh /usr/local/bin/hover-safe.sh

+# Prepare engines
+RUN hover prepare-engine --all
+ENV CGO_LDFLAGS="-L/.cache/hover/engine/linux-release -L~/.cache/hover/engine/windows-release -L~/.cache/hover/engine/darwin-release"


I think you are missing a ~ for linux

provokateurin · 2021-06-09T09:10:46Z

In the end those engines should get prepared:

darwin-debug
linux-release
linux-debug
windows-release
windows-debug

pchampio · 2021-06-09T09:16:53Z

I also have engines:

~/.cache/hover
❯ la
total 4,0K
drwxr-xr-x 1 pchampio pvhampio 1,1K 30 mai    2020 docker-go-cache
drwxr-xr-x 1 pchampio pvhampio  178 15 mars  11:32 engine
drwxr-xr-x 1 pchampio pvhampio   22 11 mai    2020 engine-profile
drwxr-xr-x 1 pchampio pvhampio   22 11 mai    2020 engine-release
-rw-r--r-- 1 pchampio pvhampio   10 21 janv. 21:50 .last_hover_check

~/.cache/hover
❯

and


~/.cache/hover/engine
❯ ll
drwxr-xr-x 1 pchampio pvhampio  82 22 mai    2020 darwin
drwxr-xr-x 1 pchampio pvhampio  72 20 mai    2020 linux
drwxr-xr-x 1 pchampio pvhampio  74 15 mars  11:32 linux-debug_unopt
drwxr-xr-x 1 pchampio pvhampio 150 22 janv. 15:15 linux-release
drwxr-xr-x 1 pchampio pvhampio  78  2 juin   2020 linux-x64
drwxr-xr-x 1 pchampio pvhampio 172  2 juin   2020 linux-x64-release
drwxr-xr-x 1 pchampio pvhampio  68 25 mai    2020 windows
drwxr-xr-x 1 pchampio pvhampio   0 21 janv. 23:51 windows-release

But I think that an older version of hover.

provokateurin · 2021-06-09T09:19:00Z

Only the ones in ~/.cache/hover/engine are used. The other ones are old.

pchampio · 2021-06-09T09:22:42Z

@jld3103 what do you think of the addition of this new command prepare-engine ?
Personally, I was thinking this PR would only be adding the :

ENV CGO_LDFLAGS=

😅
I'll prefer to have a smaller docker image that downloads the engines on demand.

pchampio · 2021-06-09T09:25:21Z

@jld3103 @cosban do you have the same error:

❯ ./install-with-docker-image.sh
[.....]
 => [flutterbuilder 2/3] RUN apt-get update     && apt-get install -y         git curl unzip                                                                                                 41.1s
 => CANCELED [pacmanbuilder 2/3] RUN apt-get update     && apt-get install -y         git meson python3 python3-pip python3-setuptools python3-wheel ninja-build gcc pkg-config m4 libarchi  68.1s
 => CANCELED [xarbuilder 2/3] RUN apt-get update  && apt-get install -y   git libssl-dev libxml2-dev make g++ autoconf zlib1g-dev                                                            68.0s
 => [appimagebuilder 2/3] RUN apt-get update  && apt-get install -y      curl                                                                                                                36.6s
 => ERROR [bomutilsbuilder 2/3] RUN apt-get update  && apt-get install -y      git make g++                                                                                                  67.9s
 => [appimagebuilder 3/3] RUN cd /opt  && curl -LO https://github.com/AppImage/AppImageKit/releases/download/continuous/appimagetool-x86_64.AppImage  && chmod a+x appimagetool-x86_64.AppIm  1.7s
 => CANCELED [flutterbuilder 3/3] RUN git clone --single-branch --depth=1 --branch beta https://github.com/flutter/flutter /opt/flutter 2>&1     && /opt/flutter/bin/flutter doctor -v       27.2s
 => CANCELED [hover  2/30] RUN apt-get update  && apt-get install -y   libgl1-mesa-dev xorg-dev   wine   genisoimage   cpio git   rpm   fakeroot bsdtar   wixl imagemagick  && rm -rf /var/  11.1s
------

[....]
#16 67.35 Unpacking patch (2.7.6-6) ...
#16 67.38 Errors were encountered while processing:
#16 67.38  /tmp/apt-dpkg-install-NJlSW2/04-perl_5.30.3-4_amd64.deb
#16 67.41 E: Sub-process /usr/bin/dpkg returned an error code (1)
------
executor failed running [/bin/sh -c apt-get update 	&& apt-get install -y 	   git make g++]: exit code: 100

provokateurin · 2021-06-09T09:25:58Z

Yeah I think a leaner image would be better, especially because the cache is mounted so they won't be downloaded every time.
I think we can leave the command so users can run it as a manual step in their CI, but remove it from our Dockerfile.
@cosban what do you think?

provokateurin · 2021-06-09T09:26:25Z

@jld3103 @cosban do you have the same error:

No, I just built it from scratch and it worked.

pchampio · 2021-06-09T09:29:21Z

Okay, I might have a proxy error.
I'll try later on another connection.

provokateurin · 2021-06-09T11:02:11Z

@cosban You have add some code so that darwin release only gets downloaded when running on darwin or on linux where darling is installed. You can probably use the code from

hover/cmd/build.go

Lines 346 to 375 in 9f74033

 func validateBuildParameters(targetOS string) { 

 if buildOrRunMode.IsAot && targetOS != runtime.GOOS && !buildIgnoreHostOS { 

 if targetOS == "windows" && runtime.GOOS != targetOS { 

 if path, err := exec.LookPath("wine"); (err != nil || len(path) == 0) && !buildOrRunDocker { 

 // Skip checking for wine if using docker, but still being on host system 

 log.Errorf("To cross-compile AOT apps for windows on %s install wine from your package manager or https://www.winehq.org/ or use the `--docker` flag", runtime.GOOS) 

 os.Exit(1) 

 } 

 } else if targetOS == "darwin" && runtime.GOOS == "linux" { 

 if buildOrRunDocker { 

 // Darling doesn't work in a docker container so it should fail when trying to use docker 

 log.Errorf("It is not possible to cross-compile AOT apps for darwin using docker") 

 log.Errorf("To cross-compile AOT apps for darwin on %s install darling from your package manager or https://www.darlinghq.org/", runtime.GOOS) 

 os.Exit(1) 

 } else if path, err := exec.LookPath("darling"); err != nil || len(path) == 0 { 

 log.Errorf("To cross-compile AOT apps for darwin on %s install darling from your package manager or https://www.darlinghq.org/", runtime.GOOS) 

 os.Exit(1) 

 } 

 } else if targetOS == "linux" { 

 if !buildOrRunDocker { 

 log.Errorf("To cross-compile AOT apps for linux on %s use the `--docker` flag", runtime.GOOS) 

 os.Exit(1) 

 } 

 } else { 

 log.Errorf("AOT builds currently only work on their host OS") 

 log.Errorf("Use the JIT release mode using the `--jit-release` flag instead") 

 os.Exit(1) 

 } 

 } 

 }

for that

provokateurin · 2021-06-09T11:09:28Z

Maybe making a common function for it is also a possibility, but idk

provokateurin · 2021-06-09T12:20:11Z

Ah well only using debug engines would also work, but then you need to change the path in the CGO_LDFLAGS too.
I'm not happy about what prepare-engine does, because --all implies that every engine gets prepared, but that is not the case.

cosban · 2021-06-09T12:45:50Z

Yeah I think a leaner image would be better, especially because the cache is mounted so they won't be downloaded every time.
I think we can leave the command so users can run it as a manual step in their CI, but remove it from our Dockerfile.
@cosban what do you think?

sounds good to me. I'll also remove the --all flag.

cosban · 2021-06-09T13:18:21Z

Maybe making a common function for it is also a possibility, but idk

It is not necessary for the validation step be copied over. It seems that the only required check from this portion is to see whether the user is trying to prepare the darwin-release engine while not on darwin, while also not having darling in the path.

remove the prepare step within the docker image, users can prepare which ever engines they need on the fly add all supported engines to the CGO_LDFLAGS of the docker image

cosban · 2021-06-09T13:27:24Z

@jld3103 what do you think of the addition of this new command prepare-engine ?
Personally, I was thinking this PR would only be adding the :
ENV CGO_LDFLAGS=
😅
I'll prefer to have a smaller docker image that downloads the engines on demand.

Is there a reason the docker image is not based on alpine if this is the case? https://hub.docker.com/_/alpine/

provokateurin · 2021-06-09T13:29:01Z

Back then there was no alpine image with golang-cross capabilities. Maybe there is one now

cosban · 2021-06-09T13:38:44Z

that's as good of a reason as any other 🤣

provokateurin · 2021-06-12T22:57:35Z

I will review this properly when I have time. Sorry :(

cosban · 2021-06-12T23:04:59Z

no worries.

cosban · 2021-06-18T14:35:45Z

Not to rush you or anything, but is there an ETA for looking at this? Would another person be able to review it before you have free time?

provokateurin · 2021-06-18T19:22:25Z

I will review it on the 20th. Sorry I'm currently busy. @pchampio could also do the review, if he wants to.

cmd/prepare-engine.go

FIX: ensure exactly one mode is set at a time

…arwin releases

cmd/prepare-engine.go

Dockerfile

cmd/prepare-engine.go

FEAT: debug mode is default again

provokateurin · 2021-06-20T19:45:46Z

LGTM, you can drop the last commit if you want to and I'll merge

cosban · 2021-06-20T20:14:15Z

I think it's fine. better to be consistent with the methodology anyway.

provokateurin · 2021-06-20T20:16:16Z

Tagged as v0.46.5

cosban added 3 commits June 8, 2021 18:37

create prepare-engine subcommand and use it in the dockerfile

a35349c

fix(deps): github.com/otiai10/[email protected]: missing go.sum entry

a02cb95

fix(deps): go mod tidy

6d289f1

cosban mentioned this pull request Jun 9, 2021

/usr/bin/ld: cannot find -lflutter_engine when attempting to test flutter plugins go-flutter-desktop/go-flutter#598

Closed

provokateurin reviewed Jun 9, 2021

View reviewed changes

cosban added 2 commits June 9, 2021 06:15

fix: swap engine prep to debug mode

35164d5

fix: add missing ~ to CGO_LDFLAGS

f7dba06

remove support for multiple target OSes in prepare-engine.

fe516df

remove the prepare step within the docker image, users can prepare which ever engines they need on the fly add all supported engines to the CGO_LDFLAGS of the docker image

cosban changed the title ~~Automatically prepare flutter engine within docker image.~~ enable users to prepare the flutter engine within docker image without building first. Jun 9, 2021

cosban requested a review from provokateurin June 9, 2021 22:55

Merge branch 'master' into master

69c2e01

provokateurin requested changes Jun 19, 2021

View reviewed changes

cmd/prepare-engine.go Outdated Show resolved Hide resolved

cmd/prepare-engine.go Show resolved Hide resolved

cmd/prepare-engine.go Outdated Show resolved Hide resolved

cosban added 3 commits June 19, 2021 20:10

FEAT: add profile mode

bf22bf8

FIX: ensure exactly one mode is set at a time

FIX: don't check for darling within prepare-engine, simply fail for d…

a8f58dc

…arwin releases

Merge branch 'master' of github.com:cosban/hover

aac022f

provokateurin requested changes Jun 20, 2021

View reviewed changes

cmd/prepare-engine.go Outdated Show resolved Hide resolved

Dockerfile Outdated Show resolved Hide resolved

cmd/prepare-engine.go Outdated Show resolved Hide resolved

cosban added 3 commits June 20, 2021 09:39

FIX: fail to prepare engine in release mode if on darwin

aa157fe

FIX: add profile for windows and linux

d3fa616

REQUEST: rework prepare mode validation logic

0d1052e

FEAT: debug mode is default again

provokateurin merged commit 5125350 into go-flutter-desktop:master Jun 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enable users to prepare the flutter engine within docker image without building first. #214

enable users to prepare the flutter engine within docker image without building first. #214

cosban commented Jun 9, 2021 •

edited

Loading

cosban commented Jun 9, 2021

provokateurin left a comment

provokateurin Jun 9, 2021

provokateurin commented Jun 9, 2021

pchampio commented Jun 9, 2021 •

edited

Loading

provokateurin commented Jun 9, 2021

pchampio commented Jun 9, 2021 •

edited

Loading

pchampio commented Jun 9, 2021

provokateurin commented Jun 9, 2021

provokateurin commented Jun 9, 2021

pchampio commented Jun 9, 2021

provokateurin commented Jun 9, 2021 •

edited

Loading

provokateurin commented Jun 9, 2021

provokateurin commented Jun 9, 2021

cosban commented Jun 9, 2021

cosban commented Jun 9, 2021

cosban commented Jun 9, 2021

provokateurin commented Jun 9, 2021 •

edited

Loading

cosban commented Jun 9, 2021

provokateurin commented Jun 12, 2021

cosban commented Jun 12, 2021

cosban commented Jun 18, 2021

provokateurin commented Jun 18, 2021

provokateurin commented Jun 20, 2021

cosban commented Jun 20, 2021

provokateurin commented Jun 20, 2021

enable users to prepare the flutter engine within docker image without building first. #214

enable users to prepare the flutter engine within docker image without building first. #214

Conversation

cosban commented Jun 9, 2021 • edited Loading

cosban commented Jun 9, 2021

provokateurin left a comment

Choose a reason for hiding this comment

provokateurin Jun 9, 2021

Choose a reason for hiding this comment

provokateurin commented Jun 9, 2021

pchampio commented Jun 9, 2021 • edited Loading

provokateurin commented Jun 9, 2021

pchampio commented Jun 9, 2021 • edited Loading

pchampio commented Jun 9, 2021

provokateurin commented Jun 9, 2021

provokateurin commented Jun 9, 2021

pchampio commented Jun 9, 2021

provokateurin commented Jun 9, 2021 • edited Loading

provokateurin commented Jun 9, 2021

provokateurin commented Jun 9, 2021

cosban commented Jun 9, 2021

cosban commented Jun 9, 2021

cosban commented Jun 9, 2021

provokateurin commented Jun 9, 2021 • edited Loading

cosban commented Jun 9, 2021

provokateurin commented Jun 12, 2021

cosban commented Jun 12, 2021

cosban commented Jun 18, 2021

provokateurin commented Jun 18, 2021

provokateurin commented Jun 20, 2021

cosban commented Jun 20, 2021

provokateurin commented Jun 20, 2021

cosban commented Jun 9, 2021 •

edited

Loading

pchampio commented Jun 9, 2021 •

edited

Loading

pchampio commented Jun 9, 2021 •

edited

Loading

provokateurin commented Jun 9, 2021 •

edited

Loading

provokateurin commented Jun 9, 2021 •

edited

Loading