openllb / hlb

A developer-first language to build and test any software efficiently
https://openllb.github.io/hlb/
Apache License 2.0
108 stars 12 forks source link

fix llb.MountOption cast #348

Closed coryb closed 1 year ago

coryb commented 1 year ago

For some reason llb.ForceNoOutput is not matching llb.MountOption in the switch statement that applies options to mounts, so just forcing the type.

After more more debuggging it seems the llb.ForceNoOutput is useful for the cases where we mount sources from external locations (http, git), this allows cached results without forcing us to awkwardly use with readonly.

Our current builds currently do something like:

fs _build() {
  image mybase
  run "go build" with option {
    mount local(".") "/src" as build
  }
}

The usage of local seems to invalidate the cache, even if the contents are the same, and note we can't apply ForceNoOutput because we bind the mount. I started a slack thread on why local invalidates the cache, but I think it will take some research to see if there are any efficient viable workarounds.

For now, the only viable workaround I have found is to force a copy, which seems to work pretty well:

fs _build() {
  image mybase
  run "go build" with option {
    mount fs {
      copy fs {
        local(".") "/src"
      } "." "."
    } "/src" as build
  }
}

The extra overhead of the copy I think is likely a much better trade-off than invaliding the run cache.

hinshun commented 1 year ago

Do we want to wrap local in a copy automatically?

coryb commented 1 year ago

Do we want to wrap local in a copy automatically?

Maybe? I think it is more optimal without the copy when we dont bind the mount (ie running something like a linter where you dont care about the file modifications), in that scenario I think local + ForceNoOuput is cacheable and optimal.

Longer term I think we likely need some form of llb.Local that allows for definition based chksums similar to llb.Image, so we can do a local scan and formulate a chksum before transferring files.

coryb commented 1 year ago

I am looking into seeing if we can test for Binding on llb.Local and to an implicit copy just in that context....

coryb commented 1 year ago

@hinshun maybe you have an idea how I can determine if we want to bind a local? Where we create the llb.Local it seems that the Binding(ctx).Bind() context is not created yet, so I always get back an empty string.

hinshun commented 1 year ago

That's because the binding is on the builtin mount, not local. Let me think about it.

hinshun commented 1 year ago

This isn't trivial because arguments are evaluated before functions, so by the time mount is invoked during codegen local is already computed. The binding is on mount and since mount is invoked after local, it's not able to pass that binding information to its arguments.

However, here you have the opportunity to detect if a CallStmt has a BindClause and pass a new WithCalleeBinding to the arguments about to be evaluated that Local can later detect: https://github.com/openllb/hlb/blob/master/codegen/codegen.go#L649

coryb commented 1 year ago

@hinshun thanks, your idea for WithCalleeBinding seems to work well, added a test to verify we get the llb.Copy implicitly injected.