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

Use non-lazy platform specific artifacts for unicode-cldr #439

Merged
merged 5 commits into from
Jul 27, 2023

Conversation

omus
Copy link
Member

@omus omus commented Jul 27, 2023

As brought up in #438 we were previously using lazy artifacts for the multiple unicode-cldr artifacts which are only needed on Windows for translating the current OS time zone name into the IANA time zone name. This approach was problematic because PackageCompiler 1.0 would always download all of the lazy artifacts and PackageCompiler 2.0 would not download the lazy artifacts (unless you use include_lazy_artifacts=true) resulting in the used unicode-cldr artifact being downloaded at runtime. Additionally, we only require the latest unicode-cldr artifact and the only reason we included multiple of these artifacts was due to us failing to clean them up in the dev/generate_artifacts.jl script which generates this file.

This PR changes the dev/generate_artifacts.jl to update the Artifacts.toml to only include the latest unicode-cldr. We now specify the platform for this architecture so that the artifact is only used on Windows which also allows us to specify it as non-lazy. Finally, there was a newer unicode-cldr release so I've also updated to use release-43-1 here.

Supercedes #438

@omus omus self-assigned this Jul 27, 2023
@omus
Copy link
Member Author

omus commented Jul 27, 2023

There is an argument to be made for testing on Windows 32-bit to ensure this artifact is present and working. I'll temporarily enable this environment as part of this PR but I don't believe it's necessary to test against this environment for other PRs. The exception being additional changes to the unicode-cldr artifact generation code.

@omus omus changed the title Use non-lazy platform specific artifact for unicode-cldr Use non-lazy platform specific artifacts for unicode-cldr Jul 27, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jul 27, 2023

Codecov Report

Merging #439 (1550176) into master (57d4a99) will not change coverage.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff           @@
##           master     #439   +/-   ##
=======================================
  Coverage   95.57%   95.57%           
=======================================
  Files          38       38           
  Lines        1762     1762           
=======================================
  Hits         1684     1684           
  Misses         78       78           
Files Changed Coverage Δ
src/winzone/WindowsTimeZoneIDs.jl 100.00% <ø> (ø)

@DilumAluthge
Copy link
Contributor

The exception being additional changes to the unicode-cldr artifact generation code.

We could configure it so that the Windows 32-bit CI job runs on PRs that touch said code, but otherwise is automatically skipped.

@omus
Copy link
Member Author

omus commented Jul 27, 2023

An additional future improvement that can be made would be to create a new artifact to replace our use of unicode-cldr such that we can use a tiny XML file as the artifact instead of the 50MB artifact we currently fetch.

@omus
Copy link
Member Author

omus commented Jul 27, 2023

The exception being additional changes to the unicode-cldr artifact generation code.

We could configure it so that the Windows 32-bit CI job runs on PRs that touch said code, but otherwise is automatically skipped.

The path filtering with GHA workflows happens at a workflow level so we'd either need a separate workflow, generate our matrix, or introduce conditions into our steps.

If I end up making the reduced sized artifact I'll probably use a separate data pkg and remove the Artifacts.toml here so I'll punt on the CI changes for now.

@omus
Copy link
Member Author

omus commented Jul 27, 2023

Validated that Windows 32-bit works in dee8df5. Reverted the commit and will merge after CI completes

@omus omus merged commit e6f7a3f into master Jul 27, 2023
15 checks passed
@omus omus deleted the cv/windows-artifacts branch July 27, 2023 03:06
Sign up for free to 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.

3 participants