Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Go rules improvements #240

Open
sluongng opened this issue May 7, 2023 · 6 comments
Open

Go rules improvements #240

sluongng opened this issue May 7, 2023 · 6 comments

Comments

@sluongng
Copy link
Contributor

sluongng commented May 7, 2023

I want to start making some contributions toward buck2 support for Go, but the existing rules left me puzzled:

  1. It's not possible to build a go_binary today. That's because there is no place in the existing rules that would include the go standard library to compile / link packages against. In Bazel's rules_go, stdlib is a stand alone action-artifact that would get included automatically into all compile / link actions.

  2. prelude//go/tools:testmaingen could not be built with the native prelude tool for the same reason as (1). So testing with go_test is also not possible. This is solved in rules_go with go_tool_binary.

    Example stacktrace:

    sluongng/remote-go-toolchain ~/work/facebook/buck2> buck2 build prelude//go/...
    File changed: prelude//toolchains/go.bzl
    File changed: root//.git/index.lock
    File changed: root//.git/index
    Action failed: prelude//go/tools:testmaingen (go_compile main)
    Local command returned non-zero exit code 2
    Reproduce locally: `env "GOARCH=arm64" "GOOS=darwin" "GOROOT=buck-out/v2/gen-anon/anon/213ed1b7ab8693791b34881e73f7aa00/ ...<omitted>... rwin_arm64/pack" @buck-out/v2/gen/prelude/213ed1b7ab869379/go/tools/__testmaingen__/main.go.argsfile (run `buck2 log what-failed` to get the full command)`
    stdout:
    prelude/go/tools/testmaingen.go:21:2: could not import bufio (file not found)
    prelude/go/tools/testmaingen.go:22:2: could not import bytes (file not found)
    prelude/go/tools/testmaingen.go:23:2: could not import errors (file not found)
    prelude/go/tools/testmaingen.go:24:2: could not import flag (file not found)
    prelude/go/tools/testmaingen.go:25:2: could not import fmt (file not found)
    prelude/go/tools/testmaingen.go:26:2: could not import go/ast (file not found)
    prelude/go/tools/testmaingen.go:27:2: could not import go/build (file not found)
    prelude/go/tools/testmaingen.go:28:2: could not import go/doc (file not found)
    prelude/go/tools/testmaingen.go:29:2: could not import go/parser (file not found)
    prelude/go/tools/testmaingen.go:30:2: could not import go/scanner (file not found)
    prelude/go/tools/testmaingen.go:30:2: too many errors
    stderr:
    Build ID: 0320ba5b-ee98-4d2c-b773-32188b23ef47
    Jobs completed: 19. Time elapsed: 0.1s. Cache hits: 0%. Commands: 2 (cached: 0, remote: 0, local: 2)
    BUILD FAILED
    Failed to build 'prelude//go/tools:testmaingen (prelude//platforms:default#213ed1b7ab869379)'
    exit 3
    
  3. It's unclear to me on how to create a language rule set to support multiple toolchains. In Bazel, we could include both the linux and darwin go toolchains in a project: darwin could be used to build locally while linux toolchain is used for remote build. Remote build could be a normal build, or could be a cross build (linux -> darwin, darwin -> linux etc...). I do see in the current Zig toolchain there is a support for target platform, but what about execution platform? Is there such a concept in Buck2?

  4. Trusted release binary. In rules_go, we would use Bazel to request the content from one of these 2 URLS

    "https://go.dev/dl/?mode=json&include=all",
    "https://golang.google.cn/dl/?mode=json&include=all",
    

    This download request does not come with a checksum, thus it will not be cached.
    They are considered "trusted" source of Go toolchain download and the json outputs include the sha256 checksum for subsequent toolchain downloads.

    However, in Buck2, the actions.download_file is more restrictive than Bazel's: it only allows you to download with a checksum. Since I could still accomplish the same thing if I were to write a bash/python binary to do all the downloads, I would suggest allowing sha256/sha1 to be optional for actions.download_file and when it's not there, simply don't cache the action outputs.

@alexlian
Copy link
Contributor

alexlian commented May 8, 2023

However, in Buck2, the actions.download_file is more restrictive than Bazel's: it only allows you to download with a checksum. Since I could still accomplish the same thing if I were to write a bash/python binary to do all the downloads, I would suggest allowing sha256/sha1 to be optional for actions.download_file and when it's not there, simply don't cache the action outputs.

I may be wrong here, but I'm understanding the premise of defining the hash is to avoid the work of downloading. If we don't know the hash a priori, we can't compare and skip the work.

@sluongng
Copy link
Contributor Author

sluongng commented May 8, 2023

However, in Buck2, the actions.download_file is more restrictive than Bazel's: it only allows you to download with a checksum. Since I could still accomplish the same thing if I were to write a bash/python binary to do all the downloads, I would suggest allowing sha256/sha1 to be optional for actions.download_file and when it's not there, simply don't cache the action outputs.

I may be wrong here, but I'm understanding the premise of defining the hash is to avoid the work of downloading. If we don't know the hash a priori, we can't compare and skip the work.

Yeah, usually the hash would be used as the cache key for such action.
In Bazel, when the sha256 is not provided for an rctx.download, then the call will simply not be cached. https://bazel.build/rules/lib/builtins/repository_ctx#download

We use it in rules_go here https://github.com/bazelbuild/rules_go/blob/c1880723e3097b788d07c8ff84ff97e58292e0e7/go/private/sdk.bzl#L84-L90 as the targeted json file does not change very frequently and the contents are mostly append-only. Once the actual go toolchain was downloaded, then the sha256 of it will exist in the cache and subsequent go targets will only need to depend on the toolchain archive itself, not the metadata used to download the archive.

It's the same as having

C depends on B
B depends on A

When building C and B exists in the cache, you would not need to check whether A is there or not.
So with

"go_binary(C)" depends on "go_toolchain(B)"
"go_toolchain(B)" depends on "toolchain_release_A.json"

You would not care about the json file most of the time.

@krallin
Copy link
Contributor

krallin commented May 9, 2023

I may be wrong here, but I'm understanding the premise of defining the hash is to avoid the work of downloading. If we don't know the hash a priori, we can't compare and skip the work.

FWIW, we also can't do that if the fingerprint doesn't match the hash algorithm we're using, and that's mostly fine.

The reason is rather that we want builds to be deterministic, and a "download this but I don't have the fingerprint" rule is clearly something that isn't deterministic.

I do think it wouldn't be unreasonable to allow opting out of fingerprinting (though internally it's probably something we'd want to disable).

@testinfected
Copy link

testinfected commented Oct 19, 2023

Hi,

Is it possible today to build a go project using Buck2? The example toolchain does not work, it seems outdated. See my comments in #239

Thanks

@ndmitchell
Copy link
Contributor

Following up in #455. We are aware Go isn't well supported in Buck2 right now - the code is there, but it requires further development. The change in Go to not supply prebuilt libraries set back the open source story, alas.

@albertocavalcante
Copy link

Which issue is tracking Go support in Buck2? I'm confused between this one and #455. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants