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

Support Winget #1625

Open
2 tasks
sitiom opened this issue Jan 26, 2023 · 13 comments · May be fixed by #1714
Open
2 tasks

Support Winget #1625

sitiom opened this issue Jan 26, 2023 · 13 comments · May be fixed by #1714
Labels
enhancement os:windows packaging Packaging and distribution

Comments

@sitiom
Copy link

sitiom commented Jan 26, 2023

Follow-up to #139 (comment):

For package managers, we are looking to the full release of winget and looking to support that.

Winget 1.4 has already been released (https://github.com/microsoft/winget-cli/releases/tag/v1.4.10173), supporting zip installation. Flyctl can now be added to the winget-pkgs repository and I have created an initial PR for it: microsoft/winget-pkgs#94699. Once this is merged, Winget Releaser has to be set up in this repository to push manifest updates automatically.

Current blockers(?):

@sitiom sitiom added the bug Something isn't working label Jan 26, 2023
@ndarilek
Copy link
Contributor

ndarilek commented Feb 7, 2023

I'm a Windows user working part-time on flyctl. This is about to become particularly interesting for me since I'm getting a new laptop and plan to use winget instead of Scoop/Chocolatey. Happy to take a crack at this from Fly's end.

I'm not familiar with how winget works internally. Does your PR need to be merged first, or do we need to install the releaser before that can happen? Either way, I'm willing to do the work on our end so I can just winget install fly soon.

@sitiom
Copy link
Author

sitiom commented Feb 8, 2023

The package has to be merged first before setting up Winget Releaser here. However, there is currently an issue with the package. It seems like flyctl is bundled with wintun.dll, but upon checking the executable dependencies via Dependencies, flyctl.exe does not seem to depend on it:
image
If flyctl depends on a DLL file, that is currently a blocker and not supported by Winget yet: microsoft/winget-cli#2711. Can you confirm if wintun.dll is needed by flyctl? If so, can you remove the unnecessary DLL in the releases file? Thanks.

@ndarilek
Copy link
Contributor

ndarilek commented Feb 8, 2023

Got it. I'm subscribed to that issue and will revisit this once it gets resolved. Definitely want to see this happen.

I'm not nearly as competent with Windows as I am with Linux. The DLL requirement seems to come from wireguard-go, which in turn depends on wintun-go, which in turn does:

func (d *lazyDLL) Load() error {
	if atomic.LoadPointer((*unsafe.Pointer)(unsafe.Pointer(&d.module))) != nil {
		return nil
	}
	d.mu.Lock()
	defer d.mu.Unlock()
	if d.module != 0 {
		return nil
	}

	const (
		LOAD_LIBRARY_SEARCH_APPLICATION_DIR = 0x00000200
		LOAD_LIBRARY_SEARCH_SYSTEM32        = 0x00000800
	)
	module, err := windows.LoadLibraryEx(d.Name, 0, LOAD_LIBRARY_SEARCH_APPLICATION_DIR|LOAD_LIBRARY_SEARCH_SYSTEM32)
	if err != nil {
		return fmt.Errorf("Unable to load library: %w", err)
	}

	atomic.StorePointer((*unsafe.Pointer)(unsafe.Pointer(&d.module)), unsafe.Pointer(module))
	if d.onLoad != nil {
		d.onLoad(d)
	}
	return nil
}

I'm not sure at what point that would make wintun.dll show up as a dependency. On launch? Whenever a WireGuard-based command is run? I'm not sure I feel comfortable removing it, particularly as I think I might be either the only Windows user on the team, or no one else wants to admit to it. :)

Thanks again.

@sitiom
Copy link
Author

sitiom commented Feb 8, 2023

Lazy-loaded DLL, yeah that's definitely an issue.

Another issue I forgot to mention is that Winget does not validate manifests with multiple aliases for a single executable (microsoft/winget-cli#2884, though I have found a workaround that is supposed to be a "bug"). Seems like that's a "niche" use case 😅 (fly and flyctl).

@ndarilek
Copy link
Contributor

ndarilek commented Feb 8, 2023 via email

@sitiom
Copy link
Author

sitiom commented Feb 8, 2023

Ah yes, no symlink support.

Aliasing does work via symlinking (the same mechanism which causes the first issue), the problem is that Winget does not successfully validate manifests with multiple symlinks/aliases for one exe, although it installs just fine.

@sitiom
Copy link
Author

sitiom commented Feb 14, 2023

@ndarilek The package is now merged to Winget, but I expect it to have some issues due to the missing wintun.dll dependency. Can you test it out once it gets refreshed on the publish pipelines?

@ndarilek
Copy link
Contributor

ndarilek commented Feb 14, 2023 via email

@sitiom
Copy link
Author

sitiom commented Feb 14, 2023

@ndarilek
Copy link
Contributor

Good news, I was able to install this build, deploy an app, and SSH into a console.

I'm new enough to flyctl that I'm not immediately sure how to test the WireGuard codepath, but I put out the call so hopefully someone can confirm conclusively that it actually works.

@sitiom
Copy link
Author

sitiom commented Feb 15, 2023

Hmm, though a few versions back, a missing wintun.dll would give out an error even in the --help command (#376). So I'm not sure what changed.

I have also tested it to be working so far as well, so I'll have to assume this is fine for now until someone reports an error in Winget. I'll now work on adding the Winget Releaser workflow in this repo (#1714). Please have a look at my PR, thanks!

@sitiom
Copy link
Author

sitiom commented Feb 15, 2023

@ndarilek Additionally, can you remove the bug label in this issue and add enhancement instead?

@sitiom sitiom linked a pull request Feb 15, 2023 that will close this issue
@ndarilek ndarilek added enhancement and removed bug Something isn't working labels Feb 15, 2023
@ndarilek
Copy link
Contributor

Done.

@michaeldwan michaeldwan added os:windows packaging Packaging and distribution labels Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement os:windows packaging Packaging and distribution
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants