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

Windows MSI #157

Closed
HaroonSaid opened this issue Mar 12, 2022 · 27 comments · Fixed by #560
Closed

Windows MSI #157

HaroonSaid opened this issue Mar 12, 2022 · 27 comments · Fixed by #560
Labels

Comments

@HaroonSaid
Copy link

Is there a windows msi installer release for the collector?

@jpkrohling
Copy link
Member

We used to, but not anymore since we moved to releasing with goreleaser on the open-telemetry/opentelemetry-collector-releases repostiory. We don't seem to have a volunteer to work on that.

@TylerHelmuth
Copy link
Member

@jpkrohling do we want to add an MSI back? Should we open an issue for it?

@jpkrohling
Copy link
Member

I believe there might be an issue with this already, just not sure where. But yes, we want this back. The problem from what I remember is that it requires a cert to sign the binary and we don't have it.

@dmitryax dmitryax added priority:p2 help wanted Extra attention is needed labels Mar 18, 2022
@mx-psi
Copy link
Member

mx-psi commented Jun 22, 2022

Should we transfer this to the releases repository?

@jpkrohling jpkrohling transferred this issue from open-telemetry/opentelemetry-collector Jul 4, 2022
@jpkrohling
Copy link
Member

Done

@dodegaard
Copy link

dodegaard commented Nov 8, 2023

@jpkrohling I may be willing to research and work on this. What is the build system used to create the releases? GitHub Actions seems to have a ton of steps defined.

@jpkrohling
Copy link
Member

We use goreleaser for automating our build. Look at .goreleaser.yaml at the root of this repository, and goreleaser.io for more information about the tool itself.

Our .goreleaser.yaml is auto-generated, but I'd recommend playing directly with that file at first, and then bringing the changes back to the Go files that generate it (cmd/goreleaser/internal/configure.go)

@smithclay
Copy link

Hey, just one quick note/good news: looks like goreleaser pro (version this repo uses) supports creating MSIs:

https://goreleaser.com/customization/msi/

@jpkrohling
Copy link
Member

Creating it is the easy part, maintaining is something more: it requires people who'd be willing to take care of bug reports. Ideally, that would be a maintainer, but if someone active enough volunteers to take care of this build, I think we can give it a try.

@mx-psi
Copy link
Member

mx-psi commented Feb 29, 2024

Would be great to have someone with Windows expertise listed on here https://github.com/open-telemetry/opentelemetry-collector/blob/main/docs/platform-support.md#tier-2--secondary-support

@pjanotti
Copy link
Contributor

pjanotti commented Mar 1, 2024

@mx-psi I volunteer myself for tier 2 support for Windows/amd64

@mx-psi
Copy link
Member

mx-psi commented Mar 4, 2024

Awesome! I am in favor of proceeding with this then :) cc @open-telemetry/collector-contrib-approvers

@andrzej-stencel
Copy link
Member

Let's start by updating https://github.com/open-telemetry/opentelemetry-collector-releases/tree/main/distributions#criteria-for-supported-distributions

What exactly should be updated there @TylerHelmuth? Do you mean to add the requirement to build Windows MSI package to point 7.?

Btw. to be honest I'm surprised that the requirement for brew packages is there - I don't think we publish brew packages for Otelcol core or contrib at all? Here's a related issue I found:

@TylerHelmuth
Copy link
Member

@astencel-sumo ya lets update 7 to say Windows MSIs are allowed.

I forget the origin of that brew comment, but I believe it was calling out brew specifically as something that a distribution can support if it wants to. I know there has been work to get a brew formula and there was some roadblocks but it is still desired.

@andrzej-stencel
Copy link
Member

andrzej-stencel commented Mar 18, 2024

lets update 7 to say Windows MSIs are allowed.

You're saying "allowed" but point 7 uses the word "must":

Must include the following assets (...) [unless] clearly stated which assets are skipped and why

I'll add Windows MSI to that list for now, we can separately consider if we should rephrase this point to make it less demanding. We currently don't provide brew or MSI for any distro.

EDIT: Oh I just re-read and found that phrase "Additional assets may be included", hm I'm not sure how this changes the meaning of this whole point 7 🤔

@TylerHelmuth
Copy link
Member

TylerHelmuth commented Mar 18, 2024

Ya maybe this doesn't need updated because of that line, but I think if we're gonna support Windows, providing an MSI as an expectation (unless otherwise specified) makes sense.

@andrzej-stencel
Copy link
Member

@briantist
Copy link

I would also love to see MSI support. I have built MSIs before and added support for them to existing projects, though it sounds like that won't be needed manually since you have a version of goreleaser with built-in support which is great.

@pjanotti
Copy link
Contributor

Four aspects that come to my mind related to releasing an MSI:

  1. MSI design: we need to agree on the install settings and of the MSI. I suppose it installs the collector as a Windows service running as LocalSystem and brings a default configuration file installed to %ProgramData%, the collector is installed machine-wide (i.e.: not per user installation), does NOT support side-by-side installs, etc.
  2. Tool(s) to build MSI: the preference is to go with whatever goreleaser offers, but, we should do due diligence and investigate the available options and respective pros and cons.
  3. Signing: there was an old issue regarding singing the exe but no resolution, see Signing the assembly  #169. Ideally we should sign the shipped binaries and the MSI itself.
  4. Builder support: this is out of scope in the near term, but, I would like to check if there is interest in adding this support to the builder some time after we have the MSI.

I’m planning to go ahead and open a new item on the collector repo to discuss item 1 and we can keep the conversation about 2 here.

@mx-psi
Copy link
Member

mx-psi commented Jun 4, 2024

I am going to revert #560 on #570, thus I am reopening this issue

@mx-psi
Copy link
Member

mx-psi commented Jun 4, 2024

On a new version of this PR, we should ensure the tests in PR actually test that producing an MSI works in an actual release.

@pjanotti
Copy link
Contributor

@mx-psi @jpkrohling there are two follow-ups, mentioned in the original #560 description, that I'm planning to do in a near future:

  1. MSI smoke tests: I would like to add some golang tests for the MSI doing the basic stuff: install/uninstall and validate the configuration. Currently, there are no tests running on the releases repository - it will be simpler to just add the tests to this repository.
  2. 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 file, either in this repo or by adding it as an option to the builder, to avoid this duplication.

We can close the present issue now or wait for a successful release of the MSIs.

@briantist
Copy link

MSI smoke tests: I would like to add some golang tests for the MSI doing the basic stuff: install/uninstall and validate the configuration. Currently, there are no tests running on the releases repository - it will be simpler to just add the tests to this repository.

@pjanotti I agree, I've written tests for MSI builds before in CI (GitHub Actions), it's important to test install, uninstall, reinstall, upgrade, etc. After the first release with an MSI it's possible to test for example, upgrading from current latest.

Here is an older example:

I did more extensive work last year for another project based on this but it's in a private repository and I can't share it, above still covers the gist of it.

The tests themselves happen to be written in Pester since PowerShell is convenient for testing the things that needed testing, but they don't have to be.

@mx-psi
Copy link
Member

mx-psi commented Jun 11, 2024

@pjanotti Let's either keep this open or open separate issues for the remaining steps

@pjanotti
Copy link
Contributor

@mx-psi
Copy link
Member

mx-psi commented Jun 12, 2024

Awesome, I am closing this one then :)

@mx-psi mx-psi closed this as completed Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants