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

proposal: cmd/go: enable GOCACHEPROG by default #64876

Open
danp opened this issue Dec 27, 2023 · 19 comments
Open

proposal: cmd/go: enable GOCACHEPROG by default #64876

danp opened this issue Dec 27, 2023 · 19 comments
Milestone

Comments

@danp
Copy link
Contributor

danp commented Dec 27, 2023

Opening this so it can at least be considered for 1.23.

With #59719 and #60419 we got support for GOCACHEPROG in 1.21+. However, it requires GOEXPERIMENT=gocacheprog be set at toolchain build time. This means official Go builds don't support GOCACHEPROG and that can make it difficult to use.

Based on issues such as dominikh/go-tools#1458 it seems at least Tailscale are having success with GOCACHEPROG.

Should it be supported by official Go builds, removing the need for toolchain-build-time GOEXPERIMENT=gocacheprog?

@gopherbot gopherbot added this to the Proposal milestone Dec 27, 2023
@danp
Copy link
Contributor Author

danp commented Dec 27, 2023

cc @bradfitz

@danp
Copy link
Contributor Author

danp commented Dec 27, 2023

If this feature is not ready for non-experimental use one option would be to still require GOEXPERIMENT=gocacheprog but move it from toolchain build time to toolchain invocation time.

@seankhliao seankhliao added the GoCommand cmd/go label Dec 27, 2023
@rsc
Copy link
Contributor

rsc commented Jan 18, 2024

/cc @bcmills for thoughts

@bcmills
Copy link
Contributor

bcmills commented Jan 18, 2024

Per the discussion in https://go.dev/cl/c/go/+/486715/14#message-086f5f820978ba6f6b2afc0589b397894afddf93, I think GOCACHEPROG needs proper integration tests in the main Go repo before we can consider promoting it to non-experimental status.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/556997 mentions this issue: cmd/go: add initial cacheprog integration tests

@seankhliao
Copy link
Member

it also needs docs, and preferably importable type definition

@rsc rsc changed the title proposal: promote GOCACHEPROG to be usable in official Go builds proposal: cmd/go: enable GOCACHEPROG by default Jan 24, 2024
@rsc
Copy link
Contributor

rsc commented Jan 26, 2024

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Feb 8, 2024

Placed on hold.
— rsc for the proposal review group

@danp
Copy link
Contributor Author

danp commented Feb 10, 2024

An item from https://go.dev/cl/556997:

The cacheprog bits use "object" when other parts of the cache use "output." For example in prog.go:

// ObjectID is set for Type "put" and "output-file".
ObjectID []byte `json:",omitempty"` // or nil if not used

vs the OutputID type and its use elsewhere:

// An OutputID is a cache output key, the hash of an output of a computation.
type OutputID [HashSize]byte

It would be good to get the cacheprog path using "output" instead of "object."

@bcmills
Copy link
Contributor

bcmills commented Feb 28, 2024

Another issue uncovered in testing: it appears that with GOCACHEPROG enabled we still end up using GOCACHE for fuzzing data. We should at least consider how we might address that, in case it affects the final GOCACHEPROG API.

(https://cs.opensource.google/go/go/+/master:src/cmd/go/internal/cache/prog.go;l=46-47;drc=5e3c4016a436c357a57a6f7870913c6911c6904e)

@rsc
Copy link
Contributor

rsc commented Mar 6, 2024

It seems like maybe fuzzing's use of GOCACHE for something completely different was a mistake. Perhaps that should be rethought.

@rsc
Copy link
Contributor

rsc commented Mar 13, 2024

Moving back to active now that there are pending tests.

@rsc
Copy link
Contributor

rsc commented Mar 15, 2024

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc removed the Proposal-Hold label Mar 15, 2024
@AlekSi
Copy link
Contributor

AlekSi commented Apr 11, 2024

It seems like maybe fuzzing's use of GOCACHE for something completely different was a mistake. Perhaps that should be rethought.

There is #48901 that proposed a new environment variable.

@nfi-hashicorp
Copy link

I'd really like to start experimenting more with this at Hashicorp, but having to build a custom Go toolchain is a bigger ask of our teams.

I wrote a little s3-backed POC and based on my tests, it's pretty much a no-brainer.

Let me know if I can help bump this along as I have a vested interest :)

@aclements
Copy link
Member

aclements commented Sep 18, 2024

I'm trying to separate out the proposal aspect of this from the implementation aspect of this.

From a proposal perspective:

  • It seems like people are generally happy with user interface to GOCACHEPROG and the protocol (plus it has a version handshake, so we can change it if necessary). @danp pointed out that the protocol mixes the terms "output ID" and "object ID". It's not clear to me if this is intentional or an oversight. Perhaps this just needs clearer documentation?
  • There's an open question around how this should interact with fuzzing, which continues to use the local GOCACHE directory. Based on my understanding, it's not actually a problem, it's just surprising, so this isn't a blocker for enabling this. There's an open issue about changing the fuzz output directory, which can proceed independently.

From an implementation perspective, the integration tests would clearly need to land before enabling it by default. Are there any other implementation blockers to enabling GOCACHEPROG by default?

@danp
Copy link
Contributor Author

danp commented Sep 18, 2024

Based on my understanding of Tailscale's and others' use, such as in this issue, I agree it seems folks are happy with the interface/protocol.

It's not clear to me if this is intentional or an oversight. Perhaps this just needs clearer documentation?

It'd probably be my preference to see the names aligned unless there is a strong reason not to. Perhaps @bradfitz can comment on intent?

I am not aware of any other blockers but it has been a bit since I've looked at things.

Re the tests CL: I admittedly did fizzle out on that waiting for more feedback. Happy to try and pick it up again with help.

@aclements
Copy link
Member

@bradfitz thinks "object ID" may have been a typo and he'll look into it. :)

@aclements
Copy link
Member

Have all remaining concerns about this proposal been addressed?

The proposal is to enable GOEXPERIMENT=gocacheprog by default. The details of gocacheprog itself are in #59719.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Active
Development

No branches or pull requests

8 participants