Add timezone data to System.Runtime - #83

Merged
akoeplinger merged 8 commits into
dotnet:masterfrom
tqiu8:add-timezone-data
Aug 26, 2020
Merged

Add timezone data to System.Runtime#83
akoeplinger merged 8 commits into
dotnet:masterfrom
tqiu8:add-timezone-data

Conversation

@tqiu8

Copy link
Copy Markdown
Contributor

Add downloaded timezone data from iana.org

@safernsafern left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we instead download it as part of the build of the package?

@tqiu8

Copy link
Copy Markdown
ContributorAuthor

@safern How can I do that?

@safern

Copy link
Copy Markdown
Member

Hmm good question. We would probably need a build script or an MSBuild task to do that. I guess it is fine as is at the moment, and we can open an issue as a follow up?

cc: @ViktorHofer

@ViktorHofer

Copy link
Copy Markdown
Member

Please avoid build scripts if possible and use msbuild instead. You should be able to add a target to the test data project file which then downloads and uncompresses the files.

@safern

Copy link
Copy Markdown
Member

You should be able to add a target to the test data project file which then downloads and uncompresses the files.

Yeah I also prefer MSBuild targets.

@akoeplinger

Copy link
Copy Markdown
Member

@ViktorHofer@safern I'm wondering whether this is the right repo for this data. Do we expect to branch runtime-assets into release/5.0? right now the master branch pushes to .NET 6 feeds already and we'll need this data in .NET 5.

@akoeplinger

Copy link
Copy Markdown
Member

I discussed with @mmitche offline and his recommendation was to branch runtime-assets for release/5.0.

Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TestData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TestData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TestData.csproj Outdated
…does not exist
Add System.Runtime.TimeZoneData.csproj to runtime-assets.sln
@tqiu8
tqiu8 requested a review from akoeplingerAugust 25, 2020 18:05
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadruntime-assets.sln Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
ViktorHofer
ViktorHofer previously requested changes Aug 25, 2020

@ViktorHoferViktorHofer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we actually want to check these external assets into the repository? If yes, are we allowed to do so? cc @jkotas

@akoeplinger

akoeplinger commented Aug 25, 2020

Copy link
Copy Markdown
Member

Do we actually want to check these external assets into the repository?

That's what we do for the Unicode files as well to avoid relying on a third party website during the build: https://github.com/dotnet/runtime-assets/tree/master/src/System.Private.Runtime.UnicodeData/13.0.0/ucd

If yes, are we allowed to do so?

The files are licensed under the public domain: https://data.iana.org/time-zones/tzdb/LICENSE

We'll need to update https://github.com/dotnet/runtime-assets/blob/master/THIRD-PARTY-NOTICES.TXT with that text.

@jkotas

Copy link
Copy Markdown
Member

What is the license for these files?

This should have an entry in https://github.com/dotnet/runtime-assets/blob/master/THIRD-PARTY-NOTICES.TXT

Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
@tqiu8

Copy link
Copy Markdown
ContributorAuthor

@ViktorHofer@akoeplinger Should I keep the timezone data in src/System.Runtime.TimeZoneData/zoneinfo then? Currently it's being saved to IntermediateOutputPath

Comment threadruntime-assets.sln
@akoeplinger

Copy link
Copy Markdown
Member

@tqiu8 after thinking about it a bit my preference is to only download and package the zoneinfo input files here in runtime-assets and do the zic compilation to binary as part of the dotnet/runtime WASM build. That avoids the issue of the build not working on Windows.

So basically download the area files + zone1970.tab into src/System.Runtime.TimeZoneData/data.

@ViktorHofer

ViktorHofer commented Aug 26, 2020

Copy link
Copy Markdown
Member

I would prefer not to check these input files into the repo but download them into the intermediate path. Having thousands of these checked in isn't ideal.

@akoeplinger

akoeplinger commented Aug 26, 2020

Copy link
Copy Markdown
Member

I do think we should check in the input files that we download from iana.org, it'd be a problem for sourcebuild otherwise.
This is also what we do for the Unicode files: #66

@tqiu8

Copy link
Copy Markdown
ContributorAuthor

Ok, so just to clarify. Source files from iana go to src/System.Runtime.TimeZoneData and zic compilation happens later in the runtime build.

@akoeplinger

Copy link
Copy Markdown
Member

Yes this looks fine to me. I'll discuss with Viktor about any concerns he has.

@tqiu8
tqiu8 requested a review from ViktorHoferAugust 26, 2020 19:51
@akoeplinger
akoeplinger dismissed ViktorHofer’s stale reviewAugust 26, 2020 21:07

discussed with Viktor offline

@akoeplinger

Copy link
Copy Markdown
Member

Discussed with Viktor on Teams, he understands the approach now and is fine with it.

@akoeplinger
akoeplinger merged commit 0349b47 into dotnet:masterAug 26, 2020
tqiu8 added a commit to tqiu8/runtime-assets that referenced this pull request Dec 1, 2020
Add downloaded timezone data from iana.org
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@tqiu8@safern@ViktorHofer@akoeplinger@jkotas
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Add timezone data to System.Runtime - #83

Merged
akoeplinger merged 8 commits into
dotnet:masterfrom
tqiu8:add-timezone-data
Aug 26, 2020
Merged

Add timezone data to System.Runtime#83
akoeplinger merged 8 commits into
dotnet:masterfrom
tqiu8:add-timezone-data

Conversation

@tqiu8

Copy link
Copy Markdown
Contributor

Add downloaded timezone data from iana.org

@safernsafern left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we instead download it as part of the build of the package?

@tqiu8

Copy link
Copy Markdown
ContributorAuthor

@safern How can I do that?

@safern

Copy link
Copy Markdown
Member

Hmm good question. We would probably need a build script or an MSBuild task to do that. I guess it is fine as is at the moment, and we can open an issue as a follow up?

cc: @ViktorHofer

@ViktorHofer

Copy link
Copy Markdown
Member

Please avoid build scripts if possible and use msbuild instead. You should be able to add a target to the test data project file which then downloads and uncompresses the files.

@safern

Copy link
Copy Markdown
Member

You should be able to add a target to the test data project file which then downloads and uncompresses the files.

Yeah I also prefer MSBuild targets.

@akoeplinger

Copy link
Copy Markdown
Member

@ViktorHofer@safern I'm wondering whether this is the right repo for this data. Do we expect to branch runtime-assets into release/5.0? right now the master branch pushes to .NET 6 feeds already and we'll need this data in .NET 5.

@akoeplinger

Copy link
Copy Markdown
Member

I discussed with @mmitche offline and his recommendation was to branch runtime-assets for release/5.0.

Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TestData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TestData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TestData.csproj Outdated
…does not exist
Add System.Runtime.TimeZoneData.csproj to runtime-assets.sln
@tqiu8
tqiu8 requested a review from akoeplingerAugust 25, 2020 18:05
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadruntime-assets.sln Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
ViktorHofer
ViktorHofer previously requested changes Aug 25, 2020

@ViktorHoferViktorHofer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we actually want to check these external assets into the repository? If yes, are we allowed to do so? cc @jkotas

@akoeplinger

akoeplinger commented Aug 25, 2020

Copy link
Copy Markdown
Member

Do we actually want to check these external assets into the repository?

That's what we do for the Unicode files as well to avoid relying on a third party website during the build: https://github.com/dotnet/runtime-assets/tree/master/src/System.Private.Runtime.UnicodeData/13.0.0/ucd

If yes, are we allowed to do so?

The files are licensed under the public domain: https://data.iana.org/time-zones/tzdb/LICENSE

We'll need to update https://github.com/dotnet/runtime-assets/blob/master/THIRD-PARTY-NOTICES.TXT with that text.

@jkotas

Copy link
Copy Markdown
Member

What is the license for these files?

This should have an entry in https://github.com/dotnet/runtime-assets/blob/master/THIRD-PARTY-NOTICES.TXT

Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
@tqiu8

Copy link
Copy Markdown
ContributorAuthor

@ViktorHofer@akoeplinger Should I keep the timezone data in src/System.Runtime.TimeZoneData/zoneinfo then? Currently it's being saved to IntermediateOutputPath

Comment threadruntime-assets.sln
@akoeplinger

Copy link
Copy Markdown
Member

@tqiu8 after thinking about it a bit my preference is to only download and package the zoneinfo input files here in runtime-assets and do the zic compilation to binary as part of the dotnet/runtime WASM build. That avoids the issue of the build not working on Windows.

So basically download the area files + zone1970.tab into src/System.Runtime.TimeZoneData/data.

@ViktorHofer

ViktorHofer commented Aug 26, 2020

Copy link
Copy Markdown
Member

I would prefer not to check these input files into the repo but download them into the intermediate path. Having thousands of these checked in isn't ideal.

@akoeplinger

akoeplinger commented Aug 26, 2020

Copy link
Copy Markdown
Member

I do think we should check in the input files that we download from iana.org, it'd be a problem for sourcebuild otherwise.
This is also what we do for the Unicode files: #66

@tqiu8

Copy link
Copy Markdown
ContributorAuthor

Ok, so just to clarify. Source files from iana go to src/System.Runtime.TimeZoneData and zic compilation happens later in the runtime build.

@akoeplinger

Copy link
Copy Markdown
Member

Yes this looks fine to me. I'll discuss with Viktor about any concerns he has.

@tqiu8
tqiu8 requested a review from ViktorHoferAugust 26, 2020 19:51
@akoeplinger
akoeplinger dismissed ViktorHofer’s stale reviewAugust 26, 2020 21:07

discussed with Viktor offline

@akoeplinger

Copy link
Copy Markdown
Member

Discussed with Viktor on Teams, he understands the approach now and is fine with it.

@akoeplinger
akoeplinger merged commit 0349b47 into dotnet:masterAug 26, 2020
tqiu8 added a commit to tqiu8/runtime-assets that referenced this pull request Dec 1, 2020
Add downloaded timezone data from iana.org
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@tqiu8@safern@ViktorHofer@akoeplinger@jkotas
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add timezone data to System.Runtime - #83

Merged
akoeplinger merged 8 commits into
dotnet:masterfrom
tqiu8:add-timezone-data
Aug 26, 2020
Merged

Add timezone data to System.Runtime#83
akoeplinger merged 8 commits into
dotnet:masterfrom
tqiu8:add-timezone-data

Conversation

@tqiu8

Copy link
Copy Markdown
Contributor

Add downloaded timezone data from iana.org

@safernsafern left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we instead download it as part of the build of the package?

@tqiu8

Copy link
Copy Markdown
ContributorAuthor

@safern How can I do that?

@safern

Copy link
Copy Markdown
Member

Hmm good question. We would probably need a build script or an MSBuild task to do that. I guess it is fine as is at the moment, and we can open an issue as a follow up?

cc: @ViktorHofer

@ViktorHofer

Copy link
Copy Markdown
Member

Please avoid build scripts if possible and use msbuild instead. You should be able to add a target to the test data project file which then downloads and uncompresses the files.

@safern

Copy link
Copy Markdown
Member

You should be able to add a target to the test data project file which then downloads and uncompresses the files.

Yeah I also prefer MSBuild targets.

@akoeplinger

Copy link
Copy Markdown
Member

@ViktorHofer@safern I'm wondering whether this is the right repo for this data. Do we expect to branch runtime-assets into release/5.0? right now the master branch pushes to .NET 6 feeds already and we'll need this data in .NET 5.

@akoeplinger

Copy link
Copy Markdown
Member

I discussed with @mmitche offline and his recommendation was to branch runtime-assets for release/5.0.

Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TestData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TestData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TestData.csproj Outdated
…does not exist
Add System.Runtime.TimeZoneData.csproj to runtime-assets.sln
@tqiu8
tqiu8 requested a review from akoeplingerAugust 25, 2020 18:05
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadruntime-assets.sln Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
ViktorHofer
ViktorHofer previously requested changes Aug 25, 2020

@ViktorHoferViktorHofer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we actually want to check these external assets into the repository? If yes, are we allowed to do so? cc @jkotas

@akoeplinger

akoeplinger commented Aug 25, 2020

Copy link
Copy Markdown
Member

Do we actually want to check these external assets into the repository?

That's what we do for the Unicode files as well to avoid relying on a third party website during the build: https://github.com/dotnet/runtime-assets/tree/master/src/System.Private.Runtime.UnicodeData/13.0.0/ucd

If yes, are we allowed to do so?

The files are licensed under the public domain: https://data.iana.org/time-zones/tzdb/LICENSE

We'll need to update https://github.com/dotnet/runtime-assets/blob/master/THIRD-PARTY-NOTICES.TXT with that text.

@jkotas

Copy link
Copy Markdown
Member

What is the license for these files?

This should have an entry in https://github.com/dotnet/runtime-assets/blob/master/THIRD-PARTY-NOTICES.TXT

Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
@tqiu8

Copy link
Copy Markdown
ContributorAuthor

@ViktorHofer@akoeplinger Should I keep the timezone data in src/System.Runtime.TimeZoneData/zoneinfo then? Currently it's being saved to IntermediateOutputPath

Comment threadruntime-assets.sln
@akoeplinger

Copy link
Copy Markdown
Member

@tqiu8 after thinking about it a bit my preference is to only download and package the zoneinfo input files here in runtime-assets and do the zic compilation to binary as part of the dotnet/runtime WASM build. That avoids the issue of the build not working on Windows.

So basically download the area files + zone1970.tab into src/System.Runtime.TimeZoneData/data.

@ViktorHofer

ViktorHofer commented Aug 26, 2020

Copy link
Copy Markdown
Member

I would prefer not to check these input files into the repo but download them into the intermediate path. Having thousands of these checked in isn't ideal.

@akoeplinger

akoeplinger commented Aug 26, 2020

Copy link
Copy Markdown
Member

I do think we should check in the input files that we download from iana.org, it'd be a problem for sourcebuild otherwise.
This is also what we do for the Unicode files: #66

@tqiu8

Copy link
Copy Markdown
ContributorAuthor

Ok, so just to clarify. Source files from iana go to src/System.Runtime.TimeZoneData and zic compilation happens later in the runtime build.

@akoeplinger

Copy link
Copy Markdown
Member

Yes this looks fine to me. I'll discuss with Viktor about any concerns he has.

@tqiu8
tqiu8 requested a review from ViktorHoferAugust 26, 2020 19:51
@akoeplinger
akoeplinger dismissed ViktorHofer’s stale reviewAugust 26, 2020 21:07

discussed with Viktor offline

@akoeplinger

Copy link
Copy Markdown
Member

Discussed with Viktor on Teams, he understands the approach now and is fine with it.

@akoeplinger
akoeplinger merged commit 0349b47 into dotnet:masterAug 26, 2020
tqiu8 added a commit to tqiu8/runtime-assets that referenced this pull request Dec 1, 2020
Add downloaded timezone data from iana.org
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@tqiu8@safern@ViktorHofer@akoeplinger@jkotas
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add timezone data to System.Runtime - #83

Merged
akoeplinger merged 8 commits into
dotnet:masterfrom
tqiu8:add-timezone-data
Aug 26, 2020
Merged

Add timezone data to System.Runtime#83
akoeplinger merged 8 commits into
dotnet:masterfrom
tqiu8:add-timezone-data

Conversation

@tqiu8

Copy link
Copy Markdown
Contributor

Add downloaded timezone data from iana.org

@safernsafern left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we instead download it as part of the build of the package?

@tqiu8

Copy link
Copy Markdown
ContributorAuthor

@safern How can I do that?

@safern

Copy link
Copy Markdown
Member

Hmm good question. We would probably need a build script or an MSBuild task to do that. I guess it is fine as is at the moment, and we can open an issue as a follow up?

cc: @ViktorHofer

@ViktorHofer

Copy link
Copy Markdown
Member

Please avoid build scripts if possible and use msbuild instead. You should be able to add a target to the test data project file which then downloads and uncompresses the files.

@safern

Copy link
Copy Markdown
Member

You should be able to add a target to the test data project file which then downloads and uncompresses the files.

Yeah I also prefer MSBuild targets.

@akoeplinger

Copy link
Copy Markdown
Member

@ViktorHofer@safern I'm wondering whether this is the right repo for this data. Do we expect to branch runtime-assets into release/5.0? right now the master branch pushes to .NET 6 feeds already and we'll need this data in .NET 5.

@akoeplinger

Copy link
Copy Markdown
Member

I discussed with @mmitche offline and his recommendation was to branch runtime-assets for release/5.0.

Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TestData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TestData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TestData.csproj Outdated
…does not exist
Add System.Runtime.TimeZoneData.csproj to runtime-assets.sln
@tqiu8
tqiu8 requested a review from akoeplingerAugust 25, 2020 18:05
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadruntime-assets.sln Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
ViktorHofer
ViktorHofer previously requested changes Aug 25, 2020

@ViktorHoferViktorHofer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we actually want to check these external assets into the repository? If yes, are we allowed to do so? cc @jkotas

@akoeplinger

akoeplinger commented Aug 25, 2020

Copy link
Copy Markdown
Member

Do we actually want to check these external assets into the repository?

That's what we do for the Unicode files as well to avoid relying on a third party website during the build: https://github.com/dotnet/runtime-assets/tree/master/src/System.Private.Runtime.UnicodeData/13.0.0/ucd

If yes, are we allowed to do so?

The files are licensed under the public domain: https://data.iana.org/time-zones/tzdb/LICENSE

We'll need to update https://github.com/dotnet/runtime-assets/blob/master/THIRD-PARTY-NOTICES.TXT with that text.

@jkotas

Copy link
Copy Markdown
Member

What is the license for these files?

This should have an entry in https://github.com/dotnet/runtime-assets/blob/master/THIRD-PARTY-NOTICES.TXT

Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
@tqiu8

Copy link
Copy Markdown
ContributorAuthor

@ViktorHofer@akoeplinger Should I keep the timezone data in src/System.Runtime.TimeZoneData/zoneinfo then? Currently it's being saved to IntermediateOutputPath

Comment threadruntime-assets.sln
@akoeplinger

Copy link
Copy Markdown
Member

@tqiu8 after thinking about it a bit my preference is to only download and package the zoneinfo input files here in runtime-assets and do the zic compilation to binary as part of the dotnet/runtime WASM build. That avoids the issue of the build not working on Windows.

So basically download the area files + zone1970.tab into src/System.Runtime.TimeZoneData/data.

@ViktorHofer

ViktorHofer commented Aug 26, 2020

Copy link
Copy Markdown
Member

I would prefer not to check these input files into the repo but download them into the intermediate path. Having thousands of these checked in isn't ideal.

@akoeplinger

akoeplinger commented Aug 26, 2020

Copy link
Copy Markdown
Member

I do think we should check in the input files that we download from iana.org, it'd be a problem for sourcebuild otherwise.
This is also what we do for the Unicode files: #66

@tqiu8

Copy link
Copy Markdown
ContributorAuthor

Ok, so just to clarify. Source files from iana go to src/System.Runtime.TimeZoneData and zic compilation happens later in the runtime build.

@akoeplinger

Copy link
Copy Markdown
Member

Yes this looks fine to me. I'll discuss with Viktor about any concerns he has.

@tqiu8
tqiu8 requested a review from ViktorHoferAugust 26, 2020 19:51
@akoeplinger
akoeplinger dismissed ViktorHofer’s stale reviewAugust 26, 2020 21:07

discussed with Viktor offline

@akoeplinger

Copy link
Copy Markdown
Member

Discussed with Viktor on Teams, he understands the approach now and is fine with it.

@akoeplinger
akoeplinger merged commit 0349b47 into dotnet:masterAug 26, 2020
tqiu8 added a commit to tqiu8/runtime-assets that referenced this pull request Dec 1, 2020
Add downloaded timezone data from iana.org
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@tqiu8@safern@ViktorHofer@akoeplinger@jkotas
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Add timezone data to System.Runtime - #83

Merged
akoeplinger merged 8 commits into
dotnet:masterfrom
tqiu8:add-timezone-data
Aug 26, 2020
Merged

Add timezone data to System.Runtime#83
akoeplinger merged 8 commits into
dotnet:masterfrom
tqiu8:add-timezone-data

Conversation

@tqiu8

Copy link
Copy Markdown
Contributor

Add downloaded timezone data from iana.org

@safernsafern left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we instead download it as part of the build of the package?

@tqiu8

Copy link
Copy Markdown
ContributorAuthor

@safern How can I do that?

@safern

Copy link
Copy Markdown
Member

Hmm good question. We would probably need a build script or an MSBuild task to do that. I guess it is fine as is at the moment, and we can open an issue as a follow up?

cc: @ViktorHofer

@ViktorHofer

Copy link
Copy Markdown
Member

Please avoid build scripts if possible and use msbuild instead. You should be able to add a target to the test data project file which then downloads and uncompresses the files.

@safern

Copy link
Copy Markdown
Member

You should be able to add a target to the test data project file which then downloads and uncompresses the files.

Yeah I also prefer MSBuild targets.

@akoeplinger

Copy link
Copy Markdown
Member

@ViktorHofer@safern I'm wondering whether this is the right repo for this data. Do we expect to branch runtime-assets into release/5.0? right now the master branch pushes to .NET 6 feeds already and we'll need this data in .NET 5.

@akoeplinger

Copy link
Copy Markdown
Member

I discussed with @mmitche offline and his recommendation was to branch runtime-assets for release/5.0.

Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TestData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TestData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TestData.csproj Outdated
…does not exist
Add System.Runtime.TimeZoneData.csproj to runtime-assets.sln
@tqiu8
tqiu8 requested a review from akoeplingerAugust 25, 2020 18:05
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadruntime-assets.sln Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
ViktorHofer
ViktorHofer previously requested changes Aug 25, 2020

@ViktorHoferViktorHofer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we actually want to check these external assets into the repository? If yes, are we allowed to do so? cc @jkotas

@akoeplinger

akoeplinger commented Aug 25, 2020

Copy link
Copy Markdown
Member

Do we actually want to check these external assets into the repository?

That's what we do for the Unicode files as well to avoid relying on a third party website during the build: https://github.com/dotnet/runtime-assets/tree/master/src/System.Private.Runtime.UnicodeData/13.0.0/ucd

If yes, are we allowed to do so?

The files are licensed under the public domain: https://data.iana.org/time-zones/tzdb/LICENSE

We'll need to update https://github.com/dotnet/runtime-assets/blob/master/THIRD-PARTY-NOTICES.TXT with that text.

@jkotas

Copy link
Copy Markdown
Member

What is the license for these files?

This should have an entry in https://github.com/dotnet/runtime-assets/blob/master/THIRD-PARTY-NOTICES.TXT

Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
@tqiu8

Copy link
Copy Markdown
ContributorAuthor

@ViktorHofer@akoeplinger Should I keep the timezone data in src/System.Runtime.TimeZoneData/zoneinfo then? Currently it's being saved to IntermediateOutputPath

Comment threadruntime-assets.sln
@akoeplinger

Copy link
Copy Markdown
Member

@tqiu8 after thinking about it a bit my preference is to only download and package the zoneinfo input files here in runtime-assets and do the zic compilation to binary as part of the dotnet/runtime WASM build. That avoids the issue of the build not working on Windows.

So basically download the area files + zone1970.tab into src/System.Runtime.TimeZoneData/data.

@ViktorHofer

ViktorHofer commented Aug 26, 2020

Copy link
Copy Markdown
Member

I would prefer not to check these input files into the repo but download them into the intermediate path. Having thousands of these checked in isn't ideal.

@akoeplinger

akoeplinger commented Aug 26, 2020

Copy link
Copy Markdown
Member

I do think we should check in the input files that we download from iana.org, it'd be a problem for sourcebuild otherwise.
This is also what we do for the Unicode files: #66

@tqiu8

Copy link
Copy Markdown
ContributorAuthor

Ok, so just to clarify. Source files from iana go to src/System.Runtime.TimeZoneData and zic compilation happens later in the runtime build.

@akoeplinger

Copy link
Copy Markdown
Member

Yes this looks fine to me. I'll discuss with Viktor about any concerns he has.

@tqiu8
tqiu8 requested a review from ViktorHoferAugust 26, 2020 19:51
@akoeplinger
akoeplinger dismissed ViktorHofer’s stale reviewAugust 26, 2020 21:07

discussed with Viktor offline

@akoeplinger

Copy link
Copy Markdown
Member

Discussed with Viktor on Teams, he understands the approach now and is fine with it.

@akoeplinger
akoeplinger merged commit 0349b47 into dotnet:masterAug 26, 2020
tqiu8 added a commit to tqiu8/runtime-assets that referenced this pull request Dec 1, 2020
Add downloaded timezone data from iana.org
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@tqiu8@safern@ViktorHofer@akoeplinger@jkotas
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add timezone data to System.Runtime - #83

Merged
akoeplinger merged 8 commits into
dotnet:masterfrom
tqiu8:add-timezone-data
Aug 26, 2020
Merged

Add timezone data to System.Runtime#83
akoeplinger merged 8 commits into
dotnet:masterfrom
tqiu8:add-timezone-data

Conversation

@tqiu8

Copy link
Copy Markdown
Contributor

Add downloaded timezone data from iana.org

@safernsafern left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we instead download it as part of the build of the package?

@tqiu8

Copy link
Copy Markdown
ContributorAuthor

@safern How can I do that?

@safern

Copy link
Copy Markdown
Member

Hmm good question. We would probably need a build script or an MSBuild task to do that. I guess it is fine as is at the moment, and we can open an issue as a follow up?

cc: @ViktorHofer

@ViktorHofer

Copy link
Copy Markdown
Member

Please avoid build scripts if possible and use msbuild instead. You should be able to add a target to the test data project file which then downloads and uncompresses the files.

@safern

Copy link
Copy Markdown
Member

You should be able to add a target to the test data project file which then downloads and uncompresses the files.

Yeah I also prefer MSBuild targets.

@akoeplinger

Copy link
Copy Markdown
Member

@ViktorHofer@safern I'm wondering whether this is the right repo for this data. Do we expect to branch runtime-assets into release/5.0? right now the master branch pushes to .NET 6 feeds already and we'll need this data in .NET 5.

@akoeplinger

Copy link
Copy Markdown
Member

I discussed with @mmitche offline and his recommendation was to branch runtime-assets for release/5.0.

Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TestData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TestData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TestData.csproj Outdated
…does not exist
Add System.Runtime.TimeZoneData.csproj to runtime-assets.sln
@tqiu8
tqiu8 requested a review from akoeplingerAugust 25, 2020 18:05
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadruntime-assets.sln Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
ViktorHofer
ViktorHofer previously requested changes Aug 25, 2020

@ViktorHoferViktorHofer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we actually want to check these external assets into the repository? If yes, are we allowed to do so? cc @jkotas

@akoeplinger

akoeplinger commented Aug 25, 2020

Copy link
Copy Markdown
Member

Do we actually want to check these external assets into the repository?

That's what we do for the Unicode files as well to avoid relying on a third party website during the build: https://github.com/dotnet/runtime-assets/tree/master/src/System.Private.Runtime.UnicodeData/13.0.0/ucd

If yes, are we allowed to do so?

The files are licensed under the public domain: https://data.iana.org/time-zones/tzdb/LICENSE

We'll need to update https://github.com/dotnet/runtime-assets/blob/master/THIRD-PARTY-NOTICES.TXT with that text.

@jkotas

Copy link
Copy Markdown
Member

What is the license for these files?

This should have an entry in https://github.com/dotnet/runtime-assets/blob/master/THIRD-PARTY-NOTICES.TXT

Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
@tqiu8

Copy link
Copy Markdown
ContributorAuthor

@ViktorHofer@akoeplinger Should I keep the timezone data in src/System.Runtime.TimeZoneData/zoneinfo then? Currently it's being saved to IntermediateOutputPath

Comment threadruntime-assets.sln
@akoeplinger

Copy link
Copy Markdown
Member

@tqiu8 after thinking about it a bit my preference is to only download and package the zoneinfo input files here in runtime-assets and do the zic compilation to binary as part of the dotnet/runtime WASM build. That avoids the issue of the build not working on Windows.

So basically download the area files + zone1970.tab into src/System.Runtime.TimeZoneData/data.

@ViktorHofer

ViktorHofer commented Aug 26, 2020

Copy link
Copy Markdown
Member

I would prefer not to check these input files into the repo but download them into the intermediate path. Having thousands of these checked in isn't ideal.

@akoeplinger

akoeplinger commented Aug 26, 2020

Copy link
Copy Markdown
Member

I do think we should check in the input files that we download from iana.org, it'd be a problem for sourcebuild otherwise.
This is also what we do for the Unicode files: #66

@tqiu8

Copy link
Copy Markdown
ContributorAuthor

Ok, so just to clarify. Source files from iana go to src/System.Runtime.TimeZoneData and zic compilation happens later in the runtime build.

@akoeplinger

Copy link
Copy Markdown
Member

Yes this looks fine to me. I'll discuss with Viktor about any concerns he has.

@tqiu8
tqiu8 requested a review from ViktorHoferAugust 26, 2020 19:51
@akoeplinger
akoeplinger dismissed ViktorHofer’s stale reviewAugust 26, 2020 21:07

discussed with Viktor offline

@akoeplinger

Copy link
Copy Markdown
Member

Discussed with Viktor on Teams, he understands the approach now and is fine with it.

@akoeplinger
akoeplinger merged commit 0349b47 into dotnet:masterAug 26, 2020
tqiu8 added a commit to tqiu8/runtime-assets that referenced this pull request Dec 1, 2020
Add downloaded timezone data from iana.org
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@tqiu8@safern@ViktorHofer@akoeplinger@jkotas
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Add timezone data to System.Runtime - #83

Merged
akoeplinger merged 8 commits into
dotnet:masterfrom
tqiu8:add-timezone-data
Aug 26, 2020
Merged

Add timezone data to System.Runtime#83
akoeplinger merged 8 commits into
dotnet:masterfrom
tqiu8:add-timezone-data

Conversation

@tqiu8

Copy link
Copy Markdown
Contributor

Add downloaded timezone data from iana.org

@safernsafern left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we instead download it as part of the build of the package?

@tqiu8

Copy link
Copy Markdown
ContributorAuthor

@safern How can I do that?

@safern

Copy link
Copy Markdown
Member

Hmm good question. We would probably need a build script or an MSBuild task to do that. I guess it is fine as is at the moment, and we can open an issue as a follow up?

cc: @ViktorHofer

@ViktorHofer

Copy link
Copy Markdown
Member

Please avoid build scripts if possible and use msbuild instead. You should be able to add a target to the test data project file which then downloads and uncompresses the files.

@safern

Copy link
Copy Markdown
Member

You should be able to add a target to the test data project file which then downloads and uncompresses the files.

Yeah I also prefer MSBuild targets.

@akoeplinger

Copy link
Copy Markdown
Member

@ViktorHofer@safern I'm wondering whether this is the right repo for this data. Do we expect to branch runtime-assets into release/5.0? right now the master branch pushes to .NET 6 feeds already and we'll need this data in .NET 5.

@akoeplinger

Copy link
Copy Markdown
Member

I discussed with @mmitche offline and his recommendation was to branch runtime-assets for release/5.0.

Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TestData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TestData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TestData.csproj Outdated
…does not exist
Add System.Runtime.TimeZoneData.csproj to runtime-assets.sln
@tqiu8
tqiu8 requested a review from akoeplingerAugust 25, 2020 18:05
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadruntime-assets.sln Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
ViktorHofer
ViktorHofer previously requested changes Aug 25, 2020

@ViktorHoferViktorHofer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we actually want to check these external assets into the repository? If yes, are we allowed to do so? cc @jkotas

@akoeplinger

akoeplinger commented Aug 25, 2020

Copy link
Copy Markdown
Member

Do we actually want to check these external assets into the repository?

That's what we do for the Unicode files as well to avoid relying on a third party website during the build: https://github.com/dotnet/runtime-assets/tree/master/src/System.Private.Runtime.UnicodeData/13.0.0/ucd

If yes, are we allowed to do so?

The files are licensed under the public domain: https://data.iana.org/time-zones/tzdb/LICENSE

We'll need to update https://github.com/dotnet/runtime-assets/blob/master/THIRD-PARTY-NOTICES.TXT with that text.

@jkotas

Copy link
Copy Markdown
Member

What is the license for these files?

This should have an entry in https://github.com/dotnet/runtime-assets/blob/master/THIRD-PARTY-NOTICES.TXT

Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
@tqiu8

Copy link
Copy Markdown
ContributorAuthor

@ViktorHofer@akoeplinger Should I keep the timezone data in src/System.Runtime.TimeZoneData/zoneinfo then? Currently it's being saved to IntermediateOutputPath

Comment threadruntime-assets.sln
@akoeplinger

Copy link
Copy Markdown
Member

@tqiu8 after thinking about it a bit my preference is to only download and package the zoneinfo input files here in runtime-assets and do the zic compilation to binary as part of the dotnet/runtime WASM build. That avoids the issue of the build not working on Windows.

So basically download the area files + zone1970.tab into src/System.Runtime.TimeZoneData/data.

@ViktorHofer

ViktorHofer commented Aug 26, 2020

Copy link
Copy Markdown
Member

I would prefer not to check these input files into the repo but download them into the intermediate path. Having thousands of these checked in isn't ideal.

@akoeplinger

akoeplinger commented Aug 26, 2020

Copy link
Copy Markdown
Member

I do think we should check in the input files that we download from iana.org, it'd be a problem for sourcebuild otherwise.
This is also what we do for the Unicode files: #66

@tqiu8

Copy link
Copy Markdown
ContributorAuthor

Ok, so just to clarify. Source files from iana go to src/System.Runtime.TimeZoneData and zic compilation happens later in the runtime build.

@akoeplinger

Copy link
Copy Markdown
Member

Yes this looks fine to me. I'll discuss with Viktor about any concerns he has.

@tqiu8
tqiu8 requested a review from ViktorHoferAugust 26, 2020 19:51
@akoeplinger
akoeplinger dismissed ViktorHofer’s stale reviewAugust 26, 2020 21:07

discussed with Viktor offline

@akoeplinger

Copy link
Copy Markdown
Member

Discussed with Viktor on Teams, he understands the approach now and is fine with it.

@akoeplinger
akoeplinger merged commit 0349b47 into dotnet:masterAug 26, 2020
tqiu8 added a commit to tqiu8/runtime-assets that referenced this pull request Dec 1, 2020
Add downloaded timezone data from iana.org
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@tqiu8@safern@ViktorHofer@akoeplinger@jkotas
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Add timezone data to System.Runtime - #83

Merged
akoeplinger merged 8 commits into
dotnet:masterfrom
tqiu8:add-timezone-data
Aug 26, 2020
Merged

Add timezone data to System.Runtime#83
akoeplinger merged 8 commits into
dotnet:masterfrom
tqiu8:add-timezone-data

Conversation

@tqiu8

Copy link
Copy Markdown
Contributor

Add downloaded timezone data from iana.org

@safernsafern left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we instead download it as part of the build of the package?

@tqiu8

Copy link
Copy Markdown
ContributorAuthor

@safern How can I do that?

@safern

Copy link
Copy Markdown
Member

Hmm good question. We would probably need a build script or an MSBuild task to do that. I guess it is fine as is at the moment, and we can open an issue as a follow up?

cc: @ViktorHofer

@ViktorHofer

Copy link
Copy Markdown
Member

Please avoid build scripts if possible and use msbuild instead. You should be able to add a target to the test data project file which then downloads and uncompresses the files.

@safern

Copy link
Copy Markdown
Member

You should be able to add a target to the test data project file which then downloads and uncompresses the files.

Yeah I also prefer MSBuild targets.

@akoeplinger

Copy link
Copy Markdown
Member

@ViktorHofer@safern I'm wondering whether this is the right repo for this data. Do we expect to branch runtime-assets into release/5.0? right now the master branch pushes to .NET 6 feeds already and we'll need this data in .NET 5.

@akoeplinger

Copy link
Copy Markdown
Member

I discussed with @mmitche offline and his recommendation was to branch runtime-assets for release/5.0.

Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TestData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TestData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TestData.csproj Outdated
…does not exist
Add System.Runtime.TimeZoneData.csproj to runtime-assets.sln
@tqiu8
tqiu8 requested a review from akoeplingerAugust 25, 2020 18:05
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadruntime-assets.sln Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
ViktorHofer
ViktorHofer previously requested changes Aug 25, 2020

@ViktorHoferViktorHofer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we actually want to check these external assets into the repository? If yes, are we allowed to do so? cc @jkotas

@akoeplinger

akoeplinger commented Aug 25, 2020

Copy link
Copy Markdown
Member

Do we actually want to check these external assets into the repository?

That's what we do for the Unicode files as well to avoid relying on a third party website during the build: https://github.com/dotnet/runtime-assets/tree/master/src/System.Private.Runtime.UnicodeData/13.0.0/ucd

If yes, are we allowed to do so?

The files are licensed under the public domain: https://data.iana.org/time-zones/tzdb/LICENSE

We'll need to update https://github.com/dotnet/runtime-assets/blob/master/THIRD-PARTY-NOTICES.TXT with that text.

@jkotas

Copy link
Copy Markdown
Member

What is the license for these files?

This should have an entry in https://github.com/dotnet/runtime-assets/blob/master/THIRD-PARTY-NOTICES.TXT

Comment threadsrc/System.Runtime.TimeZoneData/System.Runtime.TimeZoneData.csproj Outdated
@tqiu8

Copy link
Copy Markdown
ContributorAuthor

@ViktorHofer@akoeplinger Should I keep the timezone data in src/System.Runtime.TimeZoneData/zoneinfo then? Currently it's being saved to IntermediateOutputPath

Comment threadruntime-assets.sln
@akoeplinger

Copy link
Copy Markdown
Member

@tqiu8 after thinking about it a bit my preference is to only download and package the zoneinfo input files here in runtime-assets and do the zic compilation to binary as part of the dotnet/runtime WASM build. That avoids the issue of the build not working on Windows.

So basically download the area files + zone1970.tab into src/System.Runtime.TimeZoneData/data.

@ViktorHofer

ViktorHofer commented Aug 26, 2020

Copy link
Copy Markdown
Member

I would prefer not to check these input files into the repo but download them into the intermediate path. Having thousands of these checked in isn't ideal.

@akoeplinger

akoeplinger commented Aug 26, 2020

Copy link
Copy Markdown
Member

I do think we should check in the input files that we download from iana.org, it'd be a problem for sourcebuild otherwise.
This is also what we do for the Unicode files: #66

@tqiu8

Copy link
Copy Markdown
ContributorAuthor

Ok, so just to clarify. Source files from iana go to src/System.Runtime.TimeZoneData and zic compilation happens later in the runtime build.

@akoeplinger

Copy link
Copy Markdown
Member

Yes this looks fine to me. I'll discuss with Viktor about any concerns he has.

@tqiu8
tqiu8 requested a review from ViktorHoferAugust 26, 2020 19:51
@akoeplinger
akoeplinger dismissed ViktorHofer’s stale reviewAugust 26, 2020 21:07

discussed with Viktor offline

@akoeplinger

Copy link
Copy Markdown
Member

Discussed with Viktor on Teams, he understands the approach now and is fine with it.

@akoeplinger
akoeplinger merged commit 0349b47 into dotnet:masterAug 26, 2020
tqiu8 added a commit to tqiu8/runtime-assets that referenced this pull request Dec 1, 2020
Add downloaded timezone data from iana.org
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@tqiu8@safern@ViktorHofer@akoeplinger@jkotas