superfly / flyctl

Command line tools for fly.io services
https://fly.io
Apache License 2.0
1.36k stars 222 forks source link

Support Winget #1625

Open sitiom opened 1 year ago

sitiom commented 1 year ago

Follow-up to https://github.com/superfly/flyctl/issues/139#issuecomment-638920743:

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: https://github.com/microsoft/winget-pkgs/pull/94699. Once this is merged, Winget Releaser has to be set up in this repository to push manifest updates automatically.

Current blockers(?):

ndarilek commented 1 year ago

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 commented 1 year ago

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: https://github.com/microsoft/winget-cli/issues/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 commented 1 year ago

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 commented 1 year ago

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 (https://github.com/microsoft/winget-cli/issues/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 commented 1 year ago

Ah yes, no symlink support. I'll follow that one too. As a temporary workaround we can probably just copy the executable. It'll annoy some folks, but for them there's the old installer. :)

Thanks for helping get this sorted.

sitiom commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

Sure, is there something I can watch to know when the package is live? Or do I just keep running winget search fly until something promising appears? :)

sitiom commented 1 year ago
ndarilek commented 1 year ago

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 commented 1 year ago

Hmm, though a few versions back, a missing wintun.dll would give out an error even in the --help command (https://github.com/superfly/flyctl/issues/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 commented 1 year ago

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

ndarilek commented 1 year ago

Done.