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

Installer node matching failing unexpectedly in latest release #118

Closed
apetresc opened this issue Jul 21, 2021 · 9 comments · Fixed by #149
Closed

Installer node matching failing unexpectedly in latest release #118

apetresc opened this issue Jul 21, 2021 · 9 comments · Fixed by #149

Comments

@apetresc
Copy link

Brief description of your issue

Since the v0.3.0.3 release which changed a bunch of matching logic to fix things like #105 (thanks!), some of the more normal matchings seem to fail. Here is an example:

PS C:\Users\apetresc\src\winget\winget-pkgs> wingetcreate.exe update qBittorrent.qBittorrent -v 4.3.6 -u "https://sourceforge.net/projects/qbittorrent/files/qbittorrent-win32/qbittorrent-4.3.6/qbittorrent_4.3.6_x64_setup.exe/download" "https://sourceforge.net/projects/qbittorrent/files/qbittorrent-win32/qbittorrent-4.3.6/qbittorrent_4.3.6_setup.exe/download" -o .\manifests\q\qBittorrent\qBittorrent\4.3.6
Retrieving latest manifest for qBittorrent.qBittorrent
Downloading and parsing: https://sourceforge.net/projects/qbittorrent/files/qbittorrent-win32/qbittorrent-4.3.6/qbittorrent_4.3.6_x64_setup.exe/download...

Downloading and parsing: https://sourceforge.net/projects/qbittorrent/files/qbittorrent-win32/qbittorrent-4.3.6/qbittorrent_4.3.6_setup.exe/download...

Each new installer URL must have a match to an existing installer node based on installer type and architecture.
The following installers failed to match an existing installer node:

X86 Nullsoft installer detected from the url: https://sourceforge.net/projects/qbittorrent/files/qbittorrent-win32/qbittorrent-4.3.6/qbittorrent_4.3.6_setup.exe/download

However, as described in more detail below, this should be a trivial match against the identical 4.3.5 installer in the existing manifest.

Steps to reproduce

  1. Run
wingetcreate.exe update qBittorrent.qBittorrent -v 4.3.6 -u "https://sourceforge.net/projects/qbittorrent/files/qbittorrent-win32/qbittorrent-4.3.6/qbittorrent_4.3.6_x64_setup.exe/download" "https://sourceforge.net/projects/qbittorrent/files/qbittorrent-win32/qbittorrent-4.3.6/qbittorrent_4.3.6_setup.exe/download" -o .\manifests\q\qBittorrent\qBittorrent\4.3.6

Expected behavior

I'd expect this call to succeed since the two provided installers exactly match the existing 4.3.5 manifest:

Installers:
  - Architecture: x64
    InstallerType: nullsoft
    InstallerUrl: https://sourceforge.net/projects/qbittorrent/files/qbittorrent-win32/qbittorrent-4.3.5/qbittorrent_4.3.5_x64_setup.exe/download
    InstallerSha256: 724F12CA08716E3CB179A00DED6212DEAAC05394C52E6B3ED735DBEB73E080AB
#    ProductCode: 
    Scope: machine
    InstallerLocale: en-US
    UpgradeBehavior: install
  - Architecture: x86
    InstallerType: nullsoft
    InstallerUrl: https://sourceforge.net/projects/qbittorrent/files/qbittorrent-win32/qbittorrent-4.3.5/qbittorrent_4.3.5_setup.exe/download
    InstallerSha256: 4C77B9DC3E54E1F9A5AC2DAC9464BC971324D39474A7A44032AE714EF0FC41AA
#    ProductCode: 
    Scope: machine
    InstallerLocale: en-US
    UpgradeBehavior: install

Actual behavior

Fails as described in the summary, apparently failing to match the x86 nullsoft installer even though it's a perfect match for the 4.3.5 installer.

Environment

Windows Manifest Creator v0.3.0.3
@ghost ghost added the Needs-Triage label Jul 21, 2021
@palenshus
Copy link
Contributor

Hi Adrian, we wanted to be better-safe-than-sorry when inferring an architecture from the url, so we decided that if we matched two different architectures in the same url, we wouldn't use either. In this case, the url contains both win32 and x64, so we don't use either, and just fall back to architecture detection from the binary itself, which in this case is also x86.

I believe it's not trivially generalizable to determine which part of the url refers to the actual architecture, but if you have a suggestion, please let us know!

@apetresc
Copy link
Author

Oh, I see. The error output says:

X86 Nullsoft installer detected from the url: https://sourceforge.net/projects/qbittorrent/files/qbittorrent-win32/qbittorrent-4.3.6/qbittorrent_4.3.6_setup.exe/download

But I guess that one's the correct one, and it's failing because the other one (which does confusingly have both "win32" and "x64" in the URL, I get it) has the conflict, and it's simply not printing anything about the one that actually causes the problem?

Aside from the obvious improvements to the error logging, which never actually prints the URL that causes the problem, I agree there's probably not much more you can do heuristically that isn't just going to expose other corner cases. I guess the best you can do, short of a full TUI prompting the user to resolve the conflict, is to at least enable some flags that let the user disambiguate them. Something like

wingetcreate.exe update qBittorrent.qBittorrent -v 4.3.6 -u_x86 "http://..." -u_x64 "http://..."

Just a suggestion 🙂

@jedieaston
Copy link
Contributor

jedieaston commented Aug 3, 2021

short of a full TUI prompting the user to resolve the conflict

That wouldn't be a terrible idea though, IMO. Short of allowing the user to pipe in JSON/YAML for every installer entry (which isn't such a bad idea for CI pipelines that want to update lots of similar entries) we're going to have matching issues at some point. There's a lot of CLI apps out there (AWS CLI comes to mind, as does Git) that will spawn a TUI/more metadata if they detect the user is running it interactively, while for non interactive scenarios they'll just throw a bad exit code.

When I wrote my (admittedly extremely fragile) PowerShell library for maintaining the repo, I wrote some code to match via arch and it works 80% of the time, but that last 20% is really messy unless you can do some magic to figure out the scope of the installer. And when we have to add ARP data to each installer (microsoft/winget-cli#1073), this will become exponentially harder (as a installer can have a hypothetically infinite amount of ARP entries).

@dpprdan
Copy link
Contributor

dpprdan commented Aug 16, 2021

I am getting a related error for the pgAdmin (supposedly) x64 installer:

wingetcreate update PostgreSQL.pgAdmin --version 5.6 --urls https://ftp.postgresql.org/pub/pgadmin/pgadmin4/v5.6/windows/pgadmin4-5.6-x64.exe
Abrufen des letzten Manifests für PostgreSQL.pgAdmin
Herunterladen und analysieren: https://ftp.postgresql.org/pub/pgadmin/pgadmin4/v5.6/windows/pgadmin4-5.6-x64.exe...

The architecture detected from the binary might be different than what is specified in the installer URL for the following installer(s):

Using architecture detected from URL (X64), overriding architecture detected from binary (X86)
https://ftp.postgresql.org/pub/pgadmin/pgadmin4/v5.6/windows/pgadmin4-5.6-x64.exe

Each new installer URL must have a match to an existing installer node based on installer type and architecture.

Not sure why the binary architecture is X86, maybe a flag isn't set correctly there?

@palenshus
Copy link
Contributor

@dpprdan, we see this a lot, where the installer binary itself is x86, but the app that actually gets installed is x64. It would be nice if app owners updated their installers to be the correct arch, but meanwhile we have to work around it

@dpprdan
Copy link
Contributor

dpprdan commented Aug 20, 2021

@palenshus Thanks, I created an issue on the pgAdmin tracker. However, I am reading this now in the Inno Setup docs:

Inno Setup is a 32-bit application
https://jrsoftware.org/ishelp/index.php?topic=32vs64bitinstalls

So there is no way to change the Inno installer binary to x64, apparently? Which would mean that the corresponding wingetcreate check would have to be adjusted, right?

@palenshus
Copy link
Contributor

Hmm, I don't see that written at the page you linked to?

@dpprdan
Copy link
Contributor

dpprdan commented Aug 20, 2021

Sorry, wrong link. First line here: https://jrsoftware.org/ishelp/topic_64bitlimitations.htm

@denelon
Copy link
Contributor

denelon commented Aug 20, 2021

@dpprdan first I wanted to say I really like your GitHub alias. I hear (Dapper Dan) in my head every time I see it 😊.

TIL: "Inno Setup is a 32-bit application."

This explains several reasons why we're encountering challenges during validation associated with architecture.

Great call outs here. We've been looking at what we could do incrementally. I think in the case of a single installer node with a single URL it's reasonably straight forward (assuming the other installer metadata matches).

When we get into multiple installer nodes it's a bit more complicated. One of the feedback items that was resoundingly agreed to during the last MVP summit was not to have a "sidecar". Most of the MVPs suggested for "complex" manifests where a file would be passed into wingetcreate was that one might as well just write the manifest.

We truly want the CI/CD scenarios to be trivial to implement and the output must be deterministic. In that case, it may justify a much more complex command to get the job done. I think another potential impediment is that "anyone" may submit a PR that could alter an existing manifest. We've implemented the code for "verified developer" so we can lock changes down to an approved GitHub account for a given package, but the business process side of that is taking a bit longer to resolve.

Some of the challenges associated with the package manager are greatly reduced when a package is released as an MSIX, or an MSI, but when we work to meet developers where they are with the various packaging tools for .exe it's necessarily more complex. I don't think we have a silver bullet, but the feedback here is very helpful. Thank you so much for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants