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: LoadLocation doesn't work on windows if Go is not installed #21881

Closed
iman75 opened this issue Sep 14, 2017 · 68 comments
Closed

time: LoadLocation doesn't work on windows if Go is not installed #21881

iman75 opened this issue Sep 14, 2017 · 68 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Milestone

Comments

@iman75
Copy link

iman75 commented Sep 14, 2017

What version of Go are you using (go version)?

go version go1.8.3 windows/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

set GOARCH=amd64
set GOBIN=
set GOEXE=.exe
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=C:\Users\Iman Akabi\go
set GORACE=
set GOROOT=C:\Go
set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0
set CXX=g++
set CGO_ENABLED=1
set PKG_CONFIG=pkg-config
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2

What did you do?

https://play.golang.org/p/VuJ3ofkdk8
package main

import (
"fmt"
"time"
"log"
)

func main() {
var ParisLocation, err = time.LoadLocation("Europe/Paris")
if(err != nil) {
log.Fatal(err)
}

fmt.Println(ParisLocation)

}

I built this basic go code and generated an executable file.
I have an error when I run this executable on a windows machine without Go installed because time.LoadLocation needs to aceess to "zoneinfo.zip" located at $GOROOT/lib/time/zoneinfo.zip.

What did you expect to see?

Europe/Paris

What did you see instead?

open c:\go\lib\time\zoneinfo.zip: The system cannot find the file specified.

@ianlancetaylor ianlancetaylor changed the title time.LoadLocation doesn't work on windows if go is not installed time: LoadLocation doesn't work on windows if go is not installed Sep 14, 2017
@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Sep 14, 2017
@ianlancetaylor
Copy link
Contributor

I don't see how we can fix this, at least not without bloating the size of almost every Go binary on Windows. I'm open to suggestions.

@skirmish
Copy link

On windows couldn't it (default to|try) the (current working directory|executable location) to load the zoneinfo.zip? That way you only can include the file when it is required. Yeah, it's an extra hoop to jump through for us windows users, but I guess it's not too big of an ordeal seeing as the executable has to be distributed somehow, one extra file might not be too bad...

@alexbrainman
Copy link
Member

On windows couldn't it (default to|try) the (current working directory|executable location) to load the zoneinfo.zip?

Sounds simple enough for me.

Alex

@ianlancetaylor ianlancetaylor added the NeedsFix The path to resolution is known, but the work has not been done. label Sep 15, 2017
@glycerine
Copy link

zoneinfo.zip is a mere 366776 bytes, a small number amongst friends. I think of a typical go binary as a 10MB +/- 5MB affair, and I would happily increase that by 366KB (3% of the mean, but small compared to the standard deviation) to have my binaries be portable to windows.

@iman75 iman75 changed the title time: LoadLocation doesn't work on windows if go is not installed time: LoadLocation doesn't work on windows if Go is not installed Sep 22, 2017
@leighmcculloch
Copy link
Contributor

I don't see how we can fix this, at least not without bloating the size of almost every Go binary on Windows.

Providing the tzdata ourselves in the binary if a specific package is imported (e.g. time/zone) seems less worse to me than requiring every Windows user to install Go, which is much larger to download and is inconsistent with the static binary.

Could we duplicate the time.LoadLocation function to another package which also contains the tzdata, e.g. zone.LoadLocation. This new function would operate the same as time.LoadLocation, but would load from a tzdata file built into a new zone package. This would only introduce bloat to binaries that imported the zone package, and applications could choose to use time.LoadLocation or zone.LoadLocation. We could mark time.LoadLocation as deprecated, or at least mention in a comment on it about it's limitations and about the alternative. Having the basic time code and the zone code in separate packages would mean the bloat is only introduced if applications specifically need it.

This would probably also resolve issues #20629 and #20969.

@alexbrainman
Copy link
Member

Just an idea. On windows we use a particular Windows registry key to determine our timezone, and then we use a map that maps that string into std / dst values that are ultimately used to look in the tzdata blob. The "registry key" to "std / dst" map is hard coded in "abbrs" map in time package. This map contains 133 values at this moment. I wonder if we could strip tzdata blob to leave only the data we actually use on windows. I don't have time to try and implement this now, perhaps others do.

Alex

@nathan-osman
Copy link

It would be awesome if there was a function that we could call to pass the contents of zoneinfo.zip to the time package. That way, the caller is responsible for determining where they want to put zoneinfo.zip and they have the option of embedding its contents in the executable.

@bradfitz
Copy link
Contributor

@nathan-osman
Copy link

@bradfitz excellent, thanks! That will work perfectly.

@shabbyrobe
Copy link

shabbyrobe commented Mar 1, 2018

I was pretty astonished by this. I can trivially build and deploy binaries to Windows just like I can everywhere else, but as soon as I try to work with a timezone, my application blows up. Go's tooling lulls me into a false sense of ease upon which I grow to rely, but it's a false promise. I'd much rather a larger binary than an... ahem... time-bomb like this ticking away.

@glycerine
Copy link

All, what's the best current workaround for this? I need to distribute a windows binary, that uses time, and I'd like it not to blow up on user's boxes.

import "time/zone" would be great, eventually. But what's the solution today?

Thank you!

@bradfitz
Copy link
Contributor

bradfitz commented Mar 2, 2018

The solution is to ship the tzdata yourself (linked into your binary or elsewhere) and then call https://golang.org/pkg/time/#LoadLocationFromTZData.

Or set the %ZONEINFO% environment variable to point to your shipped zoneinfo.zip.

@glycerine
Copy link

Thanks Brad.

For others, the data that is passed to LoadLocationFromTZData is a file extracted from inside zoneinfo.zip, such as the file America/New_York, not zoneinfo.zip itself.

@shabbyrobe
Copy link

This is the quickie solution I used yesterday to get moving again: https://gist.github.com/shabbyrobe/b9a7a56e25bd8e441b7b3fe39dbb04fa

It relies on something not unlike the now-infamous https://github.com/jteeuwen/go-bindata to generate a go file with that tzfiles map referenced in the gist, which contains the base64-encoded zoneinfo.zip file I grabbed from the GOROOT of another Windows machine that was allowed to have Go installed on it (not always an option).

The code was dashed out under the duress of extreme time pressure so it's not exactly nice (and comes with absolutely no warranty whatsoever) but hopefully it can help someone else get out of the same jam.

@leighmcculloch
Copy link
Contributor

I created 4d63.com/tz because I find it more convenient to import a package than manually including a file to ride alongside an executable. It embeds Go's zoneinfo.zip and exports tz.LoadLocation(name) to load locations from the embedded tzdata.

@glycerine
Copy link

glycerine commented Mar 3, 2018

Awesome @leighmcculloch. That should really become part of the standard library (or rather just be the implementation of time.LoadLocation on Windows), but while waiting for that, its just great to have it available. In the meantime, all calls to time.LoadLocation, in order to not crash on windows/OS without zoneinfo, need to become calls to tz.LoadLocation() after
import "4d63.com/tz".

@glycerine
Copy link

I just spent 4 days debugging why my DLL would load on one windows host and crash on another. Time I won't ever get back. Yep, it was due to missing the %GOROOT%\lib\time\zoneinfo.zip file on the deploy host. Pretty please include zoneinfo.zip with the Go runtime on windows. Windows DLLs can't be distributed to non-developers as it stands in go1.12.5.

@skirmish
Copy link

@glycerine Sorry you spent so much time on that, agreed it's not the best solution, but in reading the discussions related to time zones on windows, adding useless static data to every go program on windows (especially when it's not required) isn't a great solution either. Windows being the oddity because it does time zones differently. You're really left with one of the above solutions, distribute the zoneinfo.zip file with your app and explicitly load it, or set the %ZONEINFO% environment variable to point to your shipped zoneinfo.zip. I had similar issues nearly 2 years ago, but now, I know the fix and to me it's just a windows(ism) I remember to always have to do. Also it came up more recently when I was building scratch containers for Linux too along with a few other dependencies under the hood. I guess also using https://4d63.com/tz work too, possibly a much cleaner solution.

@shabbyrobe
Copy link

shabbyrobe commented May 14, 2019

adding useless static data to every go program on windows (especially when it's not required) isn't a great solution either

It's less worse though. My experience was very similar to @glycerine.

This is one that could even sneak past a well-constructed build/test pipeline - you can do everything right and still be exposed to this gotcha simply by not realising your deployment target doesn't have the Go toolchain installed.

@zikaeroh
Copy link
Contributor

zikaeroh commented Mar 22, 2020

I think it should be up to the individual who deploys the binary to ensure tzdata is present.

With the _ import, if some library you depend on imports the tzdata package, then you're forced to take the 800K binary growth even if you plan to deploy with tzdata and would never use it. I can imagine some obscure library importing this ("we need timezone info, make sure we have it") and then importers who are unaware of this start growing as a result.

I'd personally rather see a flag to tell the compiler to include it than a package I can't opt-out of.

@glycerine
Copy link

What do people think of the approach in https://golang.org/cl/224588?

@ianlancetaylor I think that sounds great. Thank you for putting that together.

@glycerine
Copy link

I think it should be up to the individual who deploys the binary to ensure tzdata is present.

This is not realistic/too technical for a customer who installs your software. Welcome to the retail Windows software world.

@zikaeroh
Copy link
Contributor

zikaeroh commented Mar 22, 2020

This is not realistic/too technical for a customer who installs your software. Welcome to the retail Windows software world.

This is not what I meant. I meant that at build time if you know you need it, you can include it in the binary. I am not saying that it's something extra that needs to be installed; that is the current solution.

Presumably, if you have the issue where you don't have a Go installation on the customer's machine, then you are also not asking them to compile it themselves (if you did, then tzdata would have been found). You're building for that customer and can enable some flag to ensure they have tzdata.

@alexbrainman
Copy link
Member

With that CL, any program that imports the new time/tzdata package (as import _ "time/tzdata") will get an embedded copy of the tzdata database. ... This increases the size of the program by about 800K.

This will effectively means that all my programs will become 800K larger. Some developers of some package I use in my program will decide to include "time/tzdata", and I have to carry this useless extra disk space. This does not scale.

Alex

@leighmcculloch
Copy link
Contributor

I think the CL (#21881 (comment)) is an elegant way to let a developer choose to bundle backup tzdata, but I don't think it brings timezone support on Windows to feature parity with other operating systems that Go supports.

For example, if a developer working on Linux or Mac builds and ships binaries on all operating systems, as is common, they won't find out there's a problem until they get their first Windows user who reports an issue with the application having no timezones. So there's still the element of surprise and the code we non-Windows users write is still not Windows friendly by default.

Because of this I think parsing in that XML file idea (#21881 (comment)) is still the best solution that's been suggested as it brings the time package for Windows into parity with other operating systems. It also keeps tzdata where we expect it, in the operating system.

@glycerine
Copy link

Some developers of some package I use in my program will decide to include "time/tzdata", and I have to carry this useless extra disk space.

No, you are not impotent in this scenario. Simply delete the import from the library.

I doubt this will pacify these voices, but let's add to the time/tzdata package documentation the convention that only the main package should be importing time/tzdata.

That way anyone can feel free to delete such an import if they find it in a library. No guilt.

@ianlancetaylor
Copy link
Contributor

@leighmcculloch I think the time/tzdata suggestion is independent of adding support for parsing the Windows XML file. I think we should do that too. But even then there would be a use for some people for embedding the tzdata information in the binary itself.

@quenbyako
Copy link

quenbyako commented Mar 22, 2020

What do people think of the approach in https://golang.org/cl/224588?

Yes, this is awesome platform-independent solution, i like it. Technically, it can be more graceful, but pragmatically, looks pretty.

But. I wonder that if this solution will release, some guys will describe it as default solution, not problem specific. May be it is good to also edit docs? I can open pull request maybe.


Some developers of some package I use in my program will decide to include "time/tzdata", and I have to carry this useless extra disk space.

@alexbrainman is it? I agree, that extar 0.8MB is much space, but techically, you can cut imported package using cflags. but yes, every time use cflags is not a good idea. May be we can restrict use this package in others, except main?

@ianlancetaylor
Copy link
Contributor

I've filed #38017 as a proposal for a time/tzdata package. Please comment or emoji vote there about this idea. Thanks.

@leighmcculloch
Copy link
Contributor

I think the time/tzdata suggestion is independent of adding support for parsing the Windows XML file. I think we should do that too.

@ianlancetaylor I agree, sounds good! 👍

But even then there would be a use for some people for embedding the tzdata information in the binary itself.

For the use case of someone choosing to embed tzdata into their app, and not for solving this specific problem with Windows users, does it still make sense for the embedded tzdata to be only a backup? If I am choosing to embed data into an app I expect that data to be authoritative and be the only data used. I can see both cases (backup vs authoritative) being appropriate to different situations. Could we make the time/tzdata work like certificate pools and root CAs work? Any app can embed its own set of root CAs by creating a cert pool and adding that cert pool to the http package's default transport. The Go stdlib could still include the tzdata package, but I think the way it gets hooked up to the time package should allow:

  1. Making it a backup source of tzdata
  2. Making it an authoritative source
  3. Specifying a custom source (unlikely to be used?)

@ianlancetaylor
Copy link
Contributor

@leighmcculloch Can you discuss that over on the issue I just opened, #38017? Thanks.

@leighmcculloch
Copy link
Contributor

Sorry @ianlancetaylor I think we posted about the same time. I'll post over on #38017.

@dmitshur
Copy link
Contributor

Re-opening as CL 224588 was rolled back in CL 228200.

@dmitshur dmitshur reopened this Apr 13, 2020
@gopherbot
Copy link

Change https://golang.org/cl/228101 mentions this issue: time/tzdata: new package

@mappu
Copy link

mappu commented Apr 14, 2020

As per #21881 (comment) , the new package in CL 228101 is not a fix for this issue - just a workaround that is not suitable in all cases.

Please re-open this ticket,

@ianlancetaylor
Copy link
Contributor

I believe that the new time/tzdata package is a fix for this issue. And this issue has gotten lengthy and complex with 65 comments (now 66). Rather than reopening this issue, I encourage you to open a new issue for reading the XML file on Windows. Thanks.

@AndrewSav
Copy link

@mappu if you do, please link it here, so that those who are interested could follow

@ianlancetaylor
Copy link
Contributor

@mappu opened the new issue at #38453. Thanks.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Projects
None yet
Development

No branches or pull requests