-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Fix timezone tests to take DST into account #1349
Conversation
c520954
to
039fafe
Compare
@sc1f looks like they mightve moved boost again on windows 🤦 |
@timkpaine PR build successful though, which is strange - let me bump the windows job |
@timkpaine actions/runner-images#2667 welp - I will make a PR fixing this |
Honest question - why do we depend on system boost for Windows instead of building the version we want as we do for wasm? |
@texodus we don't have a docker image for windows; I think we may need the built boost and not just the headers, but I will check when I do the fix. There's no "system boost" - it's just that the image provides Boost for us as part of the toolchain. I agree though with @timkpaine below - that install_tools script would be good for Windows/Linux Python too, and that will go a long way in removing Docker images as a whole. |
@texodus if you want to make that script cross-platform. we need the built boost so not worth to rebuild in CI every time, but if we had a reasonable image set up it would probably help (and we could build the image using just things like (it should be noted that the install script would only be usable on CI, and we don't build or distribute windows wheels, so having something approximate a lay-person's install is a good thing). |
@timkpaine I made this change on the wasm workflow because I measured it was faster overall to build boost from source, as opposed to loading the Docker image, if you include cumulative build time saved from skipping the container. Ideally this will use Azure's cache to build We may not need to actually build boost completely, but AFAIK |
Thanks for the PR! |
In our suite of timezone tests, we set the timezone manually and assert that
getTimezoneOffset
is 300 (for EST). However, during Daylight Savings time (which started on Sunday, March 14th) the timezone offset is 240, and our tests did not take that into account. This PR fixes the issue and now should work for both DST and non-DST times.