Fix broken output on docker app build

[ulyssessouza]

With a function scoped os.File, next time the GC passes the instance is collected, calling the finalizer and triggering the invalidation of the FD, that cannot be used anymore.

- What I did Fix the broken output on docker app build

- How I did it Create a global variable to hold output file and prevent it to be garbage collected

- How to verify it By executing multiple times the following line (around 100 times) you will see that the last messages (Successfully built <digest> and Successfully tagged <tag>) will be shown consistently.

rm -rf blabla.dockerapp/ && docker app init blabla && docker app build -t blabla:v1 .

- Description for the changelog Fix broken output on docker app build

- A picture of a cute animal (not mandatory)

[codecov[bot]]

Codecov Report

Merging #768 into master will decrease coverage by 1.22%. The diff coverage is 57.14%.

@@            Coverage Diff             @@
##           master     #768      +/-   ##
==========================================
- Coverage   70.74%   69.51%   -1.23%     
==========================================
  Files          64       64              
  Lines        3675     3576      -99     
==========================================
- Hits         2600     2486     -114     
- Misses        731      760      +29     
+ Partials      344      330      -14

Impacted Files	Coverage Δ
internal/commands/build/build.go	60% <57.14%> (-9.71%)	:arrow_down:
internal/commands/inspect.go	21.66% <0%> (-28.34%)	:arrow_down:
types/parameters/parameters.go	92.06% <0%> (-4.77%)	:arrow_down:
internal/commands/image/inspect.go	65.3% <0%> (-4.09%)	:arrow_down:
internal/commands/image/rm.go	58.62% <0%> (-3.88%)	:arrow_down:
internal/store/bundle.go	76.41% <0%> (-0.17%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 06944d9...d26c9ad. Read the comment docs.

[ndeloof]

is there a issue logged in buildkit so they don't require an os.File ? would help track when we can get rid of this workaround

[tonistiigi]

Bit confused by this. NewFile does not duplicate the fd so as long as the FD() returns the right thing I don't see this much different. One of the few differences I see with this code is that os.File has a finalizer in go. Is there a small snippet showing the original problem?

[ulyssessouza]

@tonistiigi Thanks! You are right about the finalizer! TIL

The problem here was that as we create a new os.File inside buildImageUsingBuildx it's attached to its scope and when the GC passes it's allowed to remove it and by that, calls it's finalizer that invalidates the FD. This explains also the intermittence.

I just changed the PR to make a "file singleton" with getOutputFile creating and returning the global instance of os.File used in different parts of the file. Maybe there could be a more generic placement for that, but I would like to have a confirmation about the approach before.