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: add notimetzdata tag to disable tzdata embed #38679

Closed
ernado opened this issue Apr 26, 2020 · 7 comments
Closed

time: add notimetzdata tag to disable tzdata embed #38679

ernado opened this issue Apr 26, 2020 · 7 comments

Comments

@ernado
Copy link
Contributor

@ernado ernado commented Apr 26, 2020

Following #38017

Currently any transitive dependency that performs import _ "time/tzdata" will add 800kb to binary size, which is unfortunate and can compromise ongoing efforts to reduce binary size (e.g. https://github.com/tailscale/go by @bradfitz)

I propose to add notimetzdata tag so we can opt-out embedded tzdata and will make import _ "time/tzdata" no-op.

What do you think? 🙂

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 26, 2020

I think this is premature. The docs for the time/tzdata package are clear that it should only be imported from a main package. I don't think we need to start planning for packages that go against those docs until we start seeing whether the problem actually occurs in practice.

@un000
Copy link

@un000 un000 commented Apr 26, 2020

@ianlancetaylor

it should only be imported from a main package

The language doesn't constraints this.

But only one question is why tzdata should be bundled optionally with an import? Transitive libraries can broke an environment for an app. Tags are still good.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 26, 2020

New tag definitions add additional complexity and require additional documentation and testing. They also require additional work by users to understand what they mean. They are a fine tool when appropriate, but I see no reason to add them because of a purely speculative problem.

Also, how do you know when to use the tag? Unless you decide to use it all the time just in case, which seems unlikely for 99.9% of Go users, you'll identify the problem when you examine why your program is larger. And then you'll remove the unnecessary import, rather than using a tag.

@ernado
Copy link
Contributor Author

@ernado ernado commented Apr 26, 2020

Agree, probably it will be same to import _ "net/http/pprof" situation. This can be enforced via linter if needed.

Feel free to close this issue, thank you for fast response.

I still prefer tag-only version without another ability to change global state via imports, but probably it is not so user-friendly.

@un000
Copy link

@un000 un000 commented Apr 26, 2020

I agreed that is speculative problem.

Another examples: http.DefaultClient(which is bad pattern imho), rand.globalRand(seeds).
But there we can control them. If a library imports tzdata we can't control a binary size where it's needed.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Apr 27, 2020

The tzdata package has no API so the risk of people importing it for some symbol and accidentally getting the large side effect is less than other packages that mix side effects and API. That's probably a lesson to learn.

@ernado
Copy link
Contributor Author

@ernado ernado commented Apr 27, 2020

Sounds reasonable.

Consensus is that misuse risk is minimal, so closing.

@ernado ernado closed this Apr 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.