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

gh: Upload Windows build zip to GitHub releases #8787

Merged
merged 1 commit into from
Sep 13, 2024
Merged

gh: Upload Windows build zip to GitHub releases #8787

merged 1 commit into from
Sep 13, 2024

Conversation

wojtekmach
Copy link
Contributor

Follow-up on #8729

The workflow can be triggered like this:

$ gh workflow run -R wojtekmach/otp upload-windows-zip.yaml -f version=27.0.1

Here's an example workflow run: https://github.com/wojtekmach/otp/actions/runs/10738249662

This command can be used to attach zips to already existing releases.

There is one caveat with these zip builds. They have a runtime dependency on vc_redist.exe (#8531). This is not a problem for Windows exe installers as they ship with that and run it as part of the installation. In my personal opinion this is not a deal breaker for Windows zip builds, whoever ends up using them just needs to be aware of this (and/or #8531 is addressed). The tool I'd like to use these Windows zip builds for, https://elixir-install.org, handles that, for example.

@wojtekmach wojtekmach mentioned this pull request Sep 6, 2024
Closed
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 6, 2024 •
edited
Loading

CT Test Results

    3 files    143 suites   50m 17s ⏱️
1 592 tests 1 543 ✅ 49 💤 0 ❌
2 331 runs  2 257 ✅ 74 💤 0 ❌

Results for commit 80992c6.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

@garazdawi
Copy link
Contributor

Great! Would it be too much to ask if you could move the trigger for new releases from the sync-github-releases script to instead trigger on gh release edit events? We can keep the manual trigger for old releases, but for new ones I would prefer if it triggered on release edit.

@wojtekmach
Copy link
Contributor Author

wojtekmach commented Sep 9, 2024 •
edited
Loading

Not at all, I'm on it!

Something like this right?

on:
  release:
    types: [edited]

Just double-checking, when this event is trigger, would we already have the .exe installers on the release or do I need to download them from somewhere else like erlang.org just in case?

@rickard-green rickard-green added the team:VM Assigned to OTP team VM label Sep 9, 2024
@garazdawi
Copy link
Contributor

Yes, something like that. When the event is triggered you would need to check if there is a .exe but no .zip, then you create the .zip.

Think about it a bit more, I hope that updating a release within an action triggered on update release does not end up creating an infinite loop...

@wojtekmach
Copy link
Contributor Author

Gotcha, that makes sense. I'll do some testing and let you know when it's ready!

@wojtekmach
Copy link
Contributor Author

@garazdawi unfortunately I don't see a GitHub workflow being triggered when a release asset is uploaded. I had a:

on:
  release:

and nothing. I determined that updating the release description does trigger the event so that can be used as a workaround. Other than that I suppose we're in polling territory. Do you have any preferences or other ideas to try?

@garazdawi
Copy link
Contributor

Sigh. Since we already do polling in sync-github-release I guess that is where this has to land. I had hoped that was not the case. That script is far from simple, but all the data to sync old releases should already be there, it is just a matter of correctly triggering the correct action when it is missing. I pushed an attempt to your branch to make it work.

Could you please have a look and see if you agree and also add so that the sha of the installer is part of the .zip so that we can have some traceability.

@dgud
Copy link
Contributor

dgud commented Sep 10, 2024

Don't we need some instructions in the some README somewhere,

What the install.exe does so the user can choose to run it or not. (that pokes the registry, points out erlang icons and install paths).
And something about that the user might need to install vc_redist if not already done and so on?

@wojtekmach
Copy link
Contributor Author

@garazdawi your changes are looking good to me. I added a installer.sha256 file at the root of the zip and uploaded it to my test release https://github.com/wojtekmach/otp/releases/tag/OTP-27.0.1.

@dgud
Copy link
Contributor

dgud commented Sep 10, 2024

Add a README.txt or INSTALL.txt to the zip file along the installer.sha256 describing how to use the zip file.

I.e. Difference between the running the install.exe or not, and a warning about installing vcredist.

Maybe you could put the orig instruction file in erts/etc/win32/

@wojtekmach
Copy link
Contributor Author

@dgud good call, will do!

@wojtekmach
Copy link
Contributor Author

Btw does it even make sense to ship Install.exe? The current directory is already a valid installation. We can't install to another directory:

PS C:\Users\wojtek\src\otp_win64_27.0.1> .\Install.exe tmp
Do you want a minimal startup instead of sasl [No]:
Cannot open init file tmp\Install.ini

My understanding is all the win32 registry writing is done from NSIS-based installer anyway. So perhaps there's no need for Install.exe and Uninstall.exe in the zip?

Btw, at the moment the zip is around 130MB:

$ du -sh otp_win64_27.0.1.zip
128M    otp_win64_27.0.1.zip

the vc_redist.x64.exe is 25M:

$ du -sh vc_redist.x64.exe
25M     vc_redist.x64.exe

Should we ship it inside the zip? I didn't do that because I think it's fine for users to do it themselves, especially once documented in Install.txt or similar, however I thought I'd ask explicitly. I think the benefit of doing that would be such otp_win64_VSN.zip would be self-contained, holds everything required to install OTP offline.

@dgud
Copy link
Contributor

dgud commented Sep 10, 2024

Oh I thought that Install.exe wrote registry stuff as well, but I might remember it wrong.

It just creates the erl.ini which we don't want and thus erlang can't be used with sasl enabled then,
but I guess that few does that. And if you want that you should create your own start scripts anyway.

So maybe we should remove the Install.exe and Uninstall.exe, the Uninstall.exe is the Nsis created uninstall right?
If so that should be removed from the zip anyway.

vc_redist could be kept inside the zip as well so we get the correct one, there has been problems on windows when you installed
the wrong version of it, but that was a long time ago, and I don't really know how that works nowadays.

@wojtekmach
Copy link
Contributor Author

wojtekmach commented Sep 11, 2024 •
edited
Loading

Ok, I uploaded the latest test release: https://github.com/wojtekmach/otp/releases/download/OTP-27.0.1/otp_win64_27.0.1.zip

We have INSTALL.txt, installer.sha256, and vc_redist.exe. Install.exe, Install.ini, Uninstall.exe, and bin/erl.ini are removed.

garazdawi
garazdawi approved these changes Sep 11, 2024
View reviewed changes
Copy link
Contributor

@garazdawi garazdawi left a comment

Choose a reason for hiding this comment

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

I'm happy with these changes. If @dgud is as well, feel free to polish the git history if you want to and I'll merge when done.

@garazdawi garazdawi changed the base branch from maint to master September 11, 2024 10:28
@dgud
Copy link
Contributor

dgud commented Sep 11, 2024

Looks good to me.

@wojtekmach wojtekmach changed the base branch from master to maint September 11, 2024 10:48
@wojtekmach
Copy link
Contributor Author

Rebased and squashed. I also changed base branch to maint, please let me know if that's fine or whether I should change back to master.

@garazdawi
Copy link
Contributor

Please place it on master, as that is where workflow_dispatch workflows run.

@wojtekmach
Copy link
Contributor Author

Ah of course, one moment.

@wojtekmach wojtekmach changed the base branch from maint to master September 11, 2024 10:58
@garazdawi
Copy link
Contributor

Opened erlang/erlang-org#153 to show these zip archives on erlang.org

@wojtekmach
Copy link
Contributor Author

@garazdawi apologies if this is too presumptuous and/or obvious but if this is accepted and you'd like to backfill this to, say, OTP 25, I came up with this list of releases:

$ rtx list-all erlang | grep -E '^(25|26|27)' | grep -v rc
25.0
25.0.1
25.0.2
25.0.3
25.0.4
25.1
25.1.1
25.1.2
25.1.2.1
25.2
25.2.1
25.2.2
25.2.3
25.3
25.3.1
25.3.2
25.3.2.1
25.3.2.2
25.3.2.3
25.3.2.4
25.3.2.5
25.3.2.6
25.3.2.7
25.3.2.8
25.3.2.9
25.3.2.10
25.3.2.11
25.3.2.12
25.3.2.13
26.0
26.0.1
26.0.2
26.1
26.1.1
26.1.2
26.2
26.2.1
26.2.2
26.2.3
26.2.4
26.2.5
26.2.5.1
26.2.5.2
26.2.5.3
27.0
27.0.1

and you can trigger the workflow like this:

$ while read version; do
  echo gh workflow run -R erlang/otp upload-windows-zip.yaml -f version=$version
done <<EOF
25.0
25.0.1
25.0.2
25.0.3
25.0.4
25.1
25.1.1
25.1.2
25.1.2.1
25.2
25.2.1
25.2.2
25.2.3
25.3
25.3.1
25.3.2
25.3.2.1
25.3.2.2
25.3.2.3
25.3.2.4
25.3.2.5
25.3.2.6
25.3.2.7
25.3.2.8
25.3.2.9
25.3.2.10
25.3.2.11
25.3.2.12
25.3.2.13
26.0
26.0.1
26.0.2
26.1
26.1.1
26.1.2
26.2
26.2.1
26.2.2
26.2.3
26.2.4
26.2.5
26.2.5.1
26.2.5.2
26.2.5.3
27.0
27.0.1
EOF

@garazdawi garazdawi merged commit 5187279 into erlang:master Sep 13, 2024
15 checks passed
@garazdawi
Copy link
Contributor

Merged! Thanks!

The github-actions-sync script should backfill all releases that have installers automatically. So all releases after and including 23 will be updated.

@wojtekmach wojtekmach deleted the wm-windows-zip branch September 13, 2024 07:18
@wojtekmach
Copy link
Contributor Author

Oh, amazing!

@wojtekmach
Copy link
Contributor Author

@garazdawi i think I know what that HTTP 422 is, I think inputs should be under "inputs" key in request body json. I’ll send a patch in a minute.

@wojtekmach
Copy link
Contributor Author

Done: #8807

@garazdawi
Copy link
Contributor

https://github.com/erlang/otp/releases/tag/OTP-21.0 first .zip archive uploaded!

@garazdawi
Copy link
Contributor

It will only update 10 releases per hour, so it will take a while for it to finish.... :)

@wojtekmach
Copy link
Contributor Author

All good, thank you so much for working on this.

@wojtekmach wojtekmach mentioned this pull request Oct 29, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garazdawi garazdawi garazdawi approved these changes

Assignees

@dgud dgud

@garazdawi garazdawi

@jhogberg jhogberg

Labels
team:VM Assigned to OTP team VM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@wojtekmach @garazdawi @dgud @rickard-green @jhogberg