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.

Sign up for GitHub

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

Jump to bottom

Add zoneinfo data for System.Runtime.TimeZoneInfoTests #38219

Merged
merged 13 commits into from
Jul 7, 2020
Merged

Add zoneinfo data for System.Runtime.TimeZoneInfoTests #38219

merged 13 commits into from
Jul 7, 2020

Conversation

tqiu8
Copy link
Contributor

@tqiu8 tqiu8 commented Jun 22, 2020 •
edited
Loading

  • added --enable-zoneinfo flags to WasmRunnerTemplate.sh as well as wasm/sample/Makefile
  • added package reference to System.Runtime.TestData
  • allows all TimeZoneInfo tests with System.TimeZoneNotFoundException to pass, ~100 tests

@tqiu8
Add zoneinfo data for System.Runtime.TimeZoneInfoTests
b8f7d77 
- added --enable-zoneinfo flags to WasmRunnerTemplate.sh as well as wasm/sample/Makefile
- added package reference to System.Runtime.TestData
@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this PR. Please help me learn by adding exactly one area label.

@ghost
Copy link

ghost commented Jun 22, 2020

Tagging subscribers to this area: @safern, @ViktorHofer
Notify danmosemsft if you want to be subscribed.

@safern
Copy link
Member

safern commented Jun 25, 2020

@tqiu8 the PR to add WASM test runs in CI was just merged so you should be able to rebase on top of master and address: #38219 (comment)

Also, with this fix, are there any test failures left in System.Runtime.Tests?

@tqiu8
Copy link
Contributor Author

tqiu8 commented Jun 25, 2020

@safern I was gonna go a different route and just have the timezone data included in the runtime pack instead of using the package reference.

@safern
Copy link
Member

safern commented Jun 25, 2020

Is that going to be the end-to-end scenario for customers?

@tqiu8
Copy link
Contributor Author

tqiu8 commented Jun 25, 2020

@safern I think it's a temporary solution that will eventually be merged with katelyn's icu work. @lewing

safern
safern reviewed Jun 29, 2020
View reviewed changes
eng/Versions.props Outdated Show resolved Hide resolved
@tqiu8
Switch from loading timezonedata from runtime-assets to including dot…
ac91ed2 
…net.timezones.blat in runtime pack

Remove edits to WasmSample makefile
Remove package references to System.Runtime.TestData
@safern
Copy link
Member

safern commented Jun 29, 2020

Should we enable more test assemblies that were impacted by this?

lewing
lewing requested changes Jun 30, 2020
View reviewed changes
src/mono/wasm/runtime/library_mono.js Outdated Show resolved Hide resolved
src/mono/wasm/runtime/library_mono.js Outdated Show resolved Hide resolved
src/mono/wasm/runtime/library_mono.js Outdated Show resolved Hide resolved
@tqiu8 tqiu8 requested a review from lewing July 1, 2020 19:14
@tqiu8 @lewing
Replace prefix
bf9fdb8 
Co-authored-by: Larry Ewing <lewing@microsoft.com>
@tqiu8 tqiu8 requested a review from lewing July 2, 2020 19:01
lewing
lewing reviewed Jul 4, 2020
View reviewed changes
src/mono/wasm/runtime/library_mono.js Outdated Show resolved Hide resolved
src/mono/wasm/runtime/library_mono.js Outdated Show resolved Hide resolved
tqiu8 added 3 commits July 6, 2020 13:59
@tqiu8
Correct tests.mobile.targets
6dd3be1
@tqiu8
Pr feedback:
f91a069 
- Correct tests.mobile.targets
- Hardcode magic number
- Make folder depth variable
@tqiu8
Merge branch 'master' into add-zoneinfo-data
e968204
@tqiu8 tqiu8 merged commit 19d221b into dotnet:master Jul 7, 2020
akoeplinger
akoeplinger reviewed Jul 7, 2020
View reviewed changes
src/mono/wasm/Makefile
Comment on lines +115 to +116
timezone-data:
cp runtime/dotnet.timezones.blat $(BUILDS_BIN_DIR)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit in case you touch this again: this should ideally use the filename as the target name instead so make's dependency tracking works (right now it always copies the file even if it's already there:

$(BUILDS_BIN_DIR)/dotnet.timezones.blat: runtime/dotnet.timezones.blat
	cp runtime/dotnet.timezones.blat $@

(the $@ is a special variable that means the target filename: https://www.gnu.org/software/make/manual/html_node/Automatic-Variables.html)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good to know. i can open another pr with this. thank you!

@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@marek-safar marek-safar marek-safar left review comments

@akoeplinger akoeplinger akoeplinger left review comments

@lewing lewing lewing approved these changes

@safern safern safern approved these changes

@vargaz vargaz Awaiting requested review from vargaz

@EgorBo EgorBo Awaiting requested review from EgorBo

@steveisok steveisok Awaiting requested review from steveisok

Assignees
No one assigned
Labels
area-Infrastructure-libraries
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tqiu8 @Dotnet-GitSync-Bot @safern @lewing @marek-safar @akoeplinger