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

time/tzdata: consider switching to //go:embed #43350

Closed
dmitshur opened this issue Dec 23, 2020 · 19 comments
Closed

time/tzdata: consider switching to //go:embed #43350

dmitshur opened this issue Dec 23, 2020 · 19 comments
Assignees
Labels
FeatureRequest NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Dec 23, 2020

Go 1.16 adds the //go:embed directive. Can and should tzdata implementation be modified to embed a binary file rather than the current approach of generating a zipdata.go file with a const zipdata?

Doing so would slightly reduce the Go distribution size (#27151), because the zipdata.go file encodes 424 KB of data into a 1.4 MB byte .go file (so a saving of up to 1 MB). (If it can be arranged so the existing lib/time/zoneinfo.zip file is embedded directly, skipping a tzdata-specific copy, then the savings can be even larger.)

Would there be other benefits?

CC @ianlancetaylor, @rsc.

@dmitshur dmitshur added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. FeatureRequest labels Dec 23, 2020
@dmitshur dmitshur added this to the Backlog milestone Dec 23, 2020
@mengzhuo
Copy link
Contributor

IIRC go:embed can't embed files outside the package

@ianlancetaylor
Copy link
Contributor

We can copy the files into the package, and then embed them. They'll be there in the source code, but that's OK: they're already in the source code anyhow. (I don't know for sure that this will be a win, but it certainly seems plausible.)

@dmitshur
Copy link
Contributor Author

@mengzhuo Do you mean module, rather than package?

From https://go.googlesource.com/proposal/+/master/design/draft-embed.md:

It is an error for a pattern to match files outside the current module [...]

The lib/time/zoneinfo.zip file is currently outside the std module. I don't know if there are any external constraints preventing it from being moved (instead of copied).

@mengzhuo
Copy link
Contributor

@mengzhuo Do you mean module, rather than package?

From https://go.googlesource.com/proposal/+/master/design/draft-embed.md:

It is an error for a pattern to match files outside the current module [...]

The lib/time/zoneinfo.zip file is currently outside the std module. I don't know if there are any external constraints preventing it from being moved (instead of copied).

@dmitshur Yes you are right. I can submit a CL that move zoneinfo.zip into tzdata.

@mengzhuo
Copy link
Contributor

@mengzhuo Do you mean module, rather than package?

From https://go.googlesource.com/proposal/+/master/design/draft-embed.md:

It is an error for a pattern to match files outside the current module [...]

The lib/time/zoneinfo.zip file is currently outside the std module. I don't know if there are any external constraints preventing it from being moved (instead of copied).

I' m trying to put zoneinfo.zip in tzdata and build with -tags timetzdata and got import cycled.
Guess I had to alter compiler for this :)

package time
        imports time/tzdata
        imports embed
        imports io/fs
        imports time: import cycle not allowed

@mengzhuo
Copy link
Contributor

mengzhuo commented Jan 4, 2021

Unfortunately according to the depsRules(go/build/deps_test.go) that embed's dependency makes it impossible to set tzdata "depends" on embed.

<time/tzdata
<time

time
< io/fs
< embed

@dmitshur Maybe we could add some comment in the tzdata.

@mengzhuo
Copy link
Contributor

This can be done once embed dependency resolved.
#43217

@martin-sucha
Copy link
Contributor

Proposal #43217 was declined so it seems we can't use embed in tzdata.

@dmitshur
Copy link
Contributor Author

dmitshur commented Mar 15, 2021

Maybe there's another approach that can be considered.

When the "timetzdata" build constraint is satisfied, instead of having time import time/tzdata for side-effect to cause the timezone data to be embedded, that effect can be achieved in another internal-only way that doesn't involve importing time/tzdata directly. Then, it might become viable to import embed in time/tzdata.

@mengzhuo
Copy link
Contributor

Maybe there's another approach that can be considered.

When the "timetzdata" build constraint is satisfied, instead of having time import time/tzdata for side-effect to cause the timezone data to be embedded, that effect can be achieved in another internal-only way that doesn't involve importing time/tzdata directly. Then, it might become viable to import embed in time/tzdata.

Could you be more specific ?
I'm out of options :)

@mvdan
Copy link
Member

mvdan commented Mar 10, 2022

I had the same idea without noticing this older issue and mailed https://go-review.googlesource.com/c/go/+/389834, which works, minus the same problem that @mengzhuo mentions: the import cycle. I filed #51463 with a couple of ideas to work around the problem at the toolchain level. In the meantime, I might try workarounds that don't require modifying the toolchain, just to unblock the CL.

I realise that moving zoneinfo.zip from GOROOT/lib to GOROOT/src was rejected in the past, as "churn with no benefit", but I think it does have a significant benefit with go:embed: it would make the Go source distribution smaller, and the development of the package itself would be easier (one go generate step is removed and significantly less Go source for tools to load).

@mvdan
Copy link
Member

mvdan commented Mar 10, 2022

Update: I don't think we can work around the toolchain issue by cleverly using internal packages to let the time package reach the embedded tzdata string. The build tag still needs to somehow make time/tzdata be imported into the build, so its init function can run. And since the user is only required to import time, then the only option is the import behind a build tag we already have today.

I'm now implementing the toolchain change as proposed in #51463; follow that issue for updates.

@gopherbot
Copy link

Change https://go.dev/cl/389834 mentions this issue: time/tzdata: use go:embed for zoneinfo.zip

@gopherbot
Copy link

Change https://go.dev/cl/404435 mentions this issue: time/tzdata: encode the zipfile as a single giant string

@mvdan
Copy link
Member

mvdan commented Jun 22, 2022

I've closed #51463 in favor of this thread, as I no longer think my internal/embedlite proposal makes sense.

@rsc @ianlancetaylor do either of you have thoughts on #51463 (comment)? I'd love to agree on a route so we can implement it for 1.20.

@ianlancetaylor
Copy link
Contributor

I'm fine with having cmd/dist generate the file at build time.

@gopherbot
Copy link

Change https://go.dev/cl/455357 mentions this issue: time/tzdata: generate zip constant during cmd/dist

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Dec 7, 2022
@dmitshur dmitshur modified the milestones: Backlog, Go1.21 Dec 7, 2022
@mvdan
Copy link
Member

mvdan commented Jan 9, 2023

Looks like this will be merged for 1.21 when the tree reopens; thanks @rsc!

@gopherbot
Copy link

Change https://go.dev/cl/463155 mentions this issue: cmd/gomote: add missing entry in isGoToolDistGenerated

gopherbot pushed a commit to golang/build that referenced this issue Jan 23, 2023
A generated file (src/time/tzdata/zzipdata.go) was added in CL 455357.

Also add a test to catch any generated files added in the future.

Updates golang/go#43350.

Change-Id: I4965744c7023a68a68609506b9cdc99a6f27ea4a
Reviewed-on: https://go-review.googlesource.com/c/build/+/463155
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FeatureRequest NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

7 participants