[wasm] Add Timezone data generator as build task

[tqiu8]

No description provided.

[safern]

Should these areas be accepted as an input by the task so that in the future is controllable from outside of it via a MSBuild item?

[lewing]

This is a good start. Would it make sense to make two tasks? One that only made a bundle and that handled the timezone specific parts and to use as input to the bundle? I do think it would be useful expose the bundling logic for a general set of targets.

[Clockwork-Muse]

... what's the reason for including only these zones from this file? If it's for testing data, should we instead just have a static bundle file?

[tqiu8]

These are just the timezones that will be cached for SystemTimeZones. The original file was 19kb so we filtered it down to those timezones.

[Clockwork-Muse]

The file you downloaded includes a Makefile. I haven't investigated it, so this might be a dead end, but if it has an install target, does that make things easier? That is, does it just put all the zones into a final file, so you don't have to manually do a list of the files you want, or the areas to extract?

[marek-safar]

We cannot depend on latest data automatically. It's be better to move the data download outside of this task and set the version explicitly there

[tqiu8]

This is a good start. Would it make sense to make two tasks? One that only made a bundle and that handled the timezone specific parts and to use as input to the bundle? I do think it would be useful expose the bundling logic for a general set of targets.

Yes, I can separate this task into a TimeZone specific one that handles the downloading, compiling, and reordering of data; and another task that handles general input and bundles into a data file.

[marek-safar]

.

[lewing]

Great work! The bundler split looks good. For the versioning I think we can make things a little simpler still. The timezone data exists at https://data.iana.org/time-zones/tzdb<version>/ already unpacked so I think it makes sense to make the TimeZoneLoader task take a version property like 2020a then instead of downloading the zip you can download the individual files from https://data.iana.org/time-zones/tzdb2020a/<path> and build the bundle from that.

[radical]

Maybe rename this to something like DownloadTimeZoneData. That is generally how msbuild tasks are named.

[tqiu8]

It seems like the other build tasks are named liked WasmAppBuilder or PInvokeTableGenerator

[radical]

The mobile.tasks do seem to be named like that, but not others like in installer.tasks. IMHO, the task names should read like actions, that get executed in a target. Not a blocker though.

[radical]

Since this is a new task that we are creating, we can follow the msbuild conventions. The idea here is that a msbuild Target executes a bunch of Tasks, which are actions/code, so, you would name the Tasks like a method.
Let's change the names for the new tasks to match that.

[radical]

Why is this parameter needed at all? IIUC, this just seems to be the directory where files are temporarily downloaded. Do they get used after this task is done? If not, then I would suggest removing this, and instead creating a temporary directory, and downloading to that. And removing it at the end of the task.

[lewing]

This looks almost ready. The remaining issues are mostly around naming (I do think OutputFileName is clearer).

[radical]

LGTM! Good job, and thanks for being patient with the review feedback!

[marek-safar]

Few more comments, otherwise LGTM

[marek-safar]

Why is it Debug shouldn't this be $(Configuration)

[tqiu8]

hm I changed this to $(Configuration) and it throws:

/home/taqiu/runtime/src/mono/wasm/wasm.targets(40,5): error MSB4062: The "DownloadTimeZoneData" task could not 
be loaded from the assembly 
/home/taqiu/runtime/artifacts/bin/CreateWasmBundle/Release/net5.0/publish/CreateWasmBundle.dll. Could not load file 
or assembly '/home/taqiu/runtime/artifacts/bin/CreateWasmBundle/Release/net5.0/publish/CreateWasmBundle.dll'. The 
system cannot find the file specified. [/home/taqiu/runtime/src/libraries/src.proj]                            
/home/taqiu/runtime/src/mono/wasm/wasm.targets(40,5): error MSB4062:  Confirm that the <UsingTask> declaration is 
correct, that the assembly and all its dependencies are available, and that the task contains a public class that implements 
Microsoft.Build.Framework.ITask. [/home/taqiu/runtime/src/libraries/src.proj]  

I was following the <UsingTask TaskName="PInvokeTableGenerator" AssemblyFile="$([MSBuild]::NormalizePath('$(ArtifactsBinDir)', 'WasmAppBuilder', 'Debug', '$(NetCoreAppCurrent)', 'publish', 'WasmAppBuilder.dll'))"/>

[marek-safar]

Same as above

[marek-safar]

Suggested change

	data = EnumerateData();
	(byte[] json_bytes, MemoryStream stream) data = EnumerateData();

[marek-safar]

It might be helpful to print what it's doing in case the download takes long time

[marek-safar]

Why is this needed? You process additional files and then they are deleted?

[tqiu8]

Yes, this is just to remove extraneous files so that CreateWasmBundle doesn't end up including them in the data archive.

[tqiu8]

This method removes unnecessary files so that the subsequent CreateWasmBundle does not include them in the data archive.

[radical]

@tqiu8 Use Log.LogMessageFromText ("Downloading TimeZone data files", MessageImportance.Low); instead of CWL. This way, the message would end up in the logs correctly. Could you fix this as a follow up?