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 Windows MSI to the release #560

Merged
merged 28 commits into from
May 31, 2024
Merged

Add Windows MSI to the release #560

merged 28 commits into from
May 31, 2024

Conversation

pjanotti
Copy link
Contributor

@pjanotti pjanotti commented May 21, 2024 •
edited
Loading

Add Windows MSI installers to otelcol and otelcol-contrib. The installers follow the behavior described at open-telemetry/opentelemetry-collector#10043 (comment).

Fixes #157

Some notes about this change:

  • The GH runner needed to be upgraded to ubuntu-22.04 in order to install the latest msitools/wixl. On the previous version the support to CustomAction seems to be broken.
  • The windows-installer.wxs and opentelemetry.ico are duplicated on the otelcol and otelcol-contrib folders, they are exactly the same. I will look at generating at least the wxs either in this repo or by adding it as an option to the builder, to avoid this duplication.
  • I'm not adding tests to the MSIs at this time, however, it is desirable to at least do a smoke tests for all packages. I will raise this issue separately.

Tests:

  • I manually tested the MSIs using the defaults and some combinations of INSTALLDIR and COLLECTOR_SVC_ARGS
  • If you want to test the MSIs you can use the temporary artifacts from below:

pjanotti added 28 commits May 21, 2024 14:26
@pjanotti
Add Windows MSI to the release
6f010ac
@pjanotti
Add MSI support to generate-goreleaser
c32d476
@pjanotti
Setup wixl
a58435d
@pjanotti
Remove InstallPrivileges and InstallScope from wxs
68f396f
@pjanotti
Remove RemoveFile element
29673f5
@pjanotti
Directory is not supported on CustomAction
59da855
@pjanotti
Remove path prefix from opentelemetry.ico
ce52f3d
@pjanotti
Path to opentelemetry.ico
a16feef
@pjanotti
Use sys.SOURCEFILEDIR to locate .ico file
459cb9b
@pjanotti
Use sys.CURRENTDIR to locate ico file
cef70da
@pjanotti
Remove path not present on GH execution
fb8ed65
@pjanotti
Try to locate opentelmetry.ico
599b2cb
@pjanotti
Fix location of ico file when building MSI
2791692
@pjanotti
CURRENTDIR since SOURCEFILEDIR gives the wxs file
b02cba8
@pjanotti
Remove GH action step to locate ico file
2d78d20
@pjanotti
Temporarily duplicate windows folder for otelcol-contrib distribution
d9afe65
@pjanotti
Temporary action to upload artifacts
787dc72
@pjanotti
Register event source for the collector
5a000ac
@pjanotti
Use distribution name as service name
30ad144
@pjanotti
Better provider name to show up on the event viewer
5bfe10f
@pjanotti
Match both installer.wxs
b8ece30
@pjanotti
Add COLLECTOR_SVC_ARGS property
c60f97c
@pjanotti
Use ubuntu 22.04 in order to get latest wixl
51f3eb6 
It has proper support for CustomAction.
@pjanotti
CURRENTDIR didn't work with wixl 101
68dd254 
SOURCEFILEDIR didn'1 work with wixl 100 ...
@pjanotti
Change the location of opentelemetry.ico
dc7339f
@pjanotti
Move wxs source
3ecb3b8
@pjanotti
Use the binary name as event provider name
9c49554
@pjanotti
Remove temporary action to upload artifacts
d4395d7
@pjanotti pjanotti marked this pull request as ready for review May 30, 2024 20:57
@pjanotti pjanotti requested a review from a team May 30, 2024 20:57
@pjanotti pjanotti requested a review from MovieStoreGuy May 30, 2024 20:57
@pjanotti
Copy link
Contributor Author

After reviews, please squash when merging: I had to use CI to test various steps and there is no need to keep that history.

MovieStoreGuy
MovieStoreGuy approved these changes May 31, 2024
View reviewed changes
go.mod
@@ -5,7 +5,7 @@ go 1.21
toolchain go1.21.7

require (
github.com/goreleaser/goreleaser v1.25.1
github.com/goreleaser/goreleaser-pro v1.25.1-pro
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't realise you could directly use the pro version, interesting.

@jpkrohling jpkrohling self-requested a review May 31, 2024 09:11
@jpkrohling
Copy link
Member

I'll try to get a Windows machine to try this out. If I can't, I'll merge this as is. Thank you, @pjanotti!

jpkrohling
jpkrohling approved these changes May 31, 2024
View reviewed changes
Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

This was an interesting exercise: I installed Windows 11 in a VM on Gnome Boxes with an ISO downloaded from Microsoft, and after the rather painful setup (compared to Linux), I was able to install the MSI that I generated on my Fedora (hint: you might need the msitools package).

I'm not sure how to assess whether it's working, but I was able to confirm that the service is in the "Running" state on the services list. I wasn't able to figure out where the logs are, but I think I have enough evidence showing that the installation itself works.

@jpkrohling jpkrohling merged commit 4e8f050 into open-telemetry:main May 31, 2024
31 checks passed
@pjanotti pjanotti deleted the add-windows-msi branch May 31, 2024 20:34
@mx-psi
Copy link
Member

mx-psi commented Jun 4, 2024

I believe this is making the v0.102.0 release fail (example: https://github.com/open-telemetry/opentelemetry-collector-releases/actions/runs/9366469950/job/25784153242) with

  ⨯ release failed after 1m44s               error=msi: exec: "wixl": executable file not found in $PATH:

mx-psi added a commit that referenced this pull request Jun 4, 2024
@mx-psi
Revert "Add Windows MSI to the release (#560)"
2589f5d 
This reverts commit 4e8f050.
This was referenced Jun 4, 2024
Merged
Open
Closed
jpkrohling pushed a commit that referenced this pull request Jun 4, 2024
@mx-psi
Revert "Add Windows MSI to the release (#560)" (#570)
b8551a7 
This reverts commit 4e8f050.
@pjanotti
Copy link
Contributor Author

pjanotti commented Jun 5, 2024

Thanks @mx-psi - it seems that I need to add the wixl tool at least to ./.github/workflows/base-release.yaml the original PR just added it to ./.github/workflows/base-ci-releaser.yaml

pjanotti added a commit to pjanotti/opentelemetry-collector-releases that referenced this pull request Jun 5, 2024
@pjanotti
Reapply "Add Windows MSI to the release (open-telemetry#560)" (open-t…
6ea40f7 
…elemetry#570)

This reverts commit b8551a7.
@pjanotti pjanotti mentioned this pull request Jun 5, 2024
Merged
@mx-psi
Copy link
Member

mx-psi commented Jun 6, 2024

Redo over at #572

jpkrohling pushed a commit that referenced this pull request Jun 10, 2024
@pjanotti
Fix MSI release (#572)
414594f 
* Reapply "Add Windows MSI to the release (#560)" (#570)

This reverts commit b8551a7.

* Fix MSI release: add wixl tool to GH runner
@pjanotti pjanotti mentioned this pull request Jun 11, 2024
Open
@pjanotti pjanotti mentioned this pull request Aug 1, 2024
Merged
codeboten pushed a commit to open-telemetry/opentelemetry-collector that referenced this pull request Aug 1, 2024
@pjanotti
Make @pjanotti owner of windows/amd64 (#10788)
88adaef 
I'm volunteering myself to own the tier 2 support for windows/amd64.
This will put the windows/amd64 on par with the other tier 2 platforms
by having a specific person owning it.

Some of Windows related things that I contribute(d):

* Codeowner of various Windows related components in contrib
(pkg/winperfcounters, activedirectorydsreceiver, iisreceiver,
windowseventlogreceiver, and windowsperfcountersreceiver)
* Fixing Windows related issues (e.g.: #9042, #9689, #9726,
open-telemetry/opentelemetry-collector-contrib#30743)
* MSI on the collector releases repo
(open-telemetry/opentelemetry-collector-releases#560)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MovieStoreGuy MovieStoreGuy MovieStoreGuy approved these changes

@jpkrohling jpkrohling jpkrohling approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Windows MSI
4 participants
@pjanotti @jpkrohling @mx-psi @MovieStoreGuy