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 · 14 comments
Open

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

danp opened this issue Dec 27, 2023 · 14 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

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.

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

6 participants