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

gopass.gopass version 1.15.7 #118873

Merged
merged 3 commits into from
Sep 8, 2023
Merged

gopass.gopass version 1.15.7 #118873

merged 3 commits into from
Sep 8, 2023

Conversation

dpprdan
Copy link
Contributor

@dpprdan dpprdan commented Sep 4, 2023

  • Have you signed the Contributor License Agreement?
  • Have you checked that there aren't other open pull requests for the same manifest update/change?
  • This PR only modifies one (1) manifest
  • Have you validated your manifest locally with winget validate --manifest <path>?
  • Have you tested your manifest locally with winget install --manifest <path>?
  • Does your manifest conform to the 1.5 schema?

Note: <path> is the name of the directory containing the manifest you're submitting.


Microsoft Reviewers: Open in CodeFlow

@wingetbot
Copy link
Collaborator

Service Badge  Service Badge  

@wingetbot
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wingetbot
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@wingetbot wingetbot added Azure-Pipeline-Passed Validation pipeline passed. There may still be manual validation requirements. Validation-Executable-Error labels Sep 4, 2023
@stephengillie
Copy link
Collaborator

Manual Validation failed with:

2023-09-05 16:55:36.204 [CLI ] Installer [X86,nullsoft,Machine,] not applicable: Architecture was excluded by caller : X86 > 2023-09-05 16:55:36.205 [CLI ] No suitable installer found for manifest GnuPG.Gpg4win with version 4.2.0

It appears that the dependency GnuPG.Gpg4win doesn't have an installer set that matches the main package.

@stephengillie stephengillie added the Needs-Author-Feedback This needs a response from the author. label Sep 6, 2023
@dpprdan
Copy link
Contributor Author

dpprdan commented Sep 6, 2023

@stephengillie I'm confused

  1. What does Architecture was excluded by caller : X86 mean exactly, i.e. who is the caller? So yes, this is a x64 program requiring a x86 dependency. But why is that a problem?
  2. Why wasn't this flagged in Add gopass.gopass version 1.15.5 #106108? There are not differences between that gopass manifest and the corresponding/latest Gpg4win manifest at the time (i.e. https://github.com/microsoft/winget-pkgs/tree/816db29ca04a5661dad7cd0046848ab3406ccecc/manifests/g/GnuPG/Gpg4win/4.1.0) versus the one in this PR and https://github.com/microsoft/winget-pkgs/tree/816db29ca04a5661dad7cd0046848ab3406ccecc/manifests/g/GnuPG/Gpg4win/4.2.0.
  3. What do you mean with "Manual Validation"? AFAICT all CI checks pass, but they do not catch this error, so you have to do an additional manual validation? That sounds ... cumbersome?
    NB the manifest installed fine for me during my manual validation with SandboxTest.ps1.

As mentioned in #106108 (comment), this is not a hard dependency, i.e. you can install the gopass package without it, but you cannot meaningfully use it without an encryption backend. So a "solution" could be to just remove the dependencies, but that doesn't strike me as the most user-friendly solution. 🤷🏻‍♂️

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Attention This work item needs to be reviewed by a member of the core team. and removed Needs-Author-Feedback This needs a response from the author. labels Sep 6, 2023
@stephengillie
Copy link
Collaborator

I tried with a modified manifest. Swapped x64 for x86, and this unblocked GnuPG.Gpg4win but blocked Git.Git, which lost its 32-bit version in the past year:

2023-09-06 18:18:31.575 [CLI ] Starting installer selection.
2023-09-06 18:18:31.575 [CLI ] Installer [X64,inno,User,] not applicable: Architecture was excluded by caller : X64
2023-09-06 18:18:31.575 [CLI ] Installer [X64,inno,Machine,] not applicable: Architecture was excluded by caller : X64
2023-09-06 18:18:31.575 [CLI ] No suitable installer found for manifest Git.Git with version 2.42.0.2

  1. I'm not sure but I'm guessing that the caller might be us, as we're calling the process.
  • I believe all dependencies have to have the same architecture, installer type, and scope. This is shown in the [X64,inno,User,] list above. (The 3rd comma might indicate a 4th field that I missed.)
  1. Add gopass.gopass version 1.15.5 #106108 was merged about 6 months ago.
  • 2 or 3 versions of WinGet have been released in that time.
    • Dependency management within WinGet has had some work.
  • Git.Git wasn't a dependency in this previous version.
  1. Here is the Manual Validation script I maintain: https://github.com/microsoft/winget-pkgs/blob/master/Tools/ManualValidationPipeline.ps1
  • The process runs on Windows 10 VMs that the script also helps maintain.
  • What CI checks are you running?
  • Interesting that this installed in a Sandbox. They have ~80% of the Windows OS and so give a good picture about 80% of the time. But don't support Defender so can't complete the Validation process.

@dpprdan
Copy link
Contributor Author

dpprdan commented Sep 7, 2023

Thanks, @stephengillie!

I still don't understand why a x64 program requiring a x86 dependency is flagged as a problem? That's the basic issue here, correct?

I believe all dependencies have to have the same architecture, installer type, and scope.

If that is really the case, I don't see how winget dependency management is going to work in practice.
(NB I understand that a x86-only program requiring a x64-only dependency or a user-scope only program requiring a machine-scope-only dependency needs to block validation, but other than that I don't understand why it would need be that strict).

Relatedly, I don't understand, why I as a submitter, can only be alerted to this ☝🏻 after you run your custom/manual validation?

What CI checks are you running?

I am referring to the validation pipeline here on GH.
grafik
Why is it necessary for you to have your own additional validation script? And if it is really necessary to have these additional checks, why are they not part the CI pipeline? As a consequence, submitting a manifest drags on for days.

[Sandbox doesn't] support Defender so can't complete the Validation process

x64 vs x86 isn't a Defender problem, though, is it?

@wingetbot
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@microsoft-github-policy-service microsoft-github-policy-service bot removed Azure-Pipeline-Passed Validation pipeline passed. There may still be manual validation requirements. Validation-Executable-Error labels Sep 7, 2023
@dpprdan
Copy link
Contributor Author

dpprdan commented Sep 7, 2023

I hope removing the deps resolves this.

@wingetbot wingetbot added Azure-Pipeline-Passed Validation pipeline passed. There may still be manual validation requirements. Validation-Executable-Error labels Sep 7, 2023
@stephengillie
Copy link
Collaborator

@wingetbot waivers Add Validation-Executable-Error

@wingetbot
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@microsoft-github-policy-service microsoft-github-policy-service bot removed Azure-Pipeline-Passed Validation pipeline passed. There may still be manual validation requirements. Validation-Executable-Error labels Sep 7, 2023
@stephengillie
Copy link
Collaborator

I hope removing the deps resolves this.

Yes - it installs and launches normally.

  • The desktop shortcut doesn't appear to do anything. It flashed a window onscreen the first time and no output since. Is this expected?
PS C:\Users\User\Desktop> .\gopass.lnk
PS C:\Users\User\Desktop> C:\Users\User\AppData\Local\gopass\gopass.exe

   __     _    _ _      _ _   ___   ___
 /'_ '\ /'_'\ ( '_'\  /'_' )/',__)/',__)
( (_) |( (_) )| (_) )( (_| |\__, \\__, \
'\__  |'\___/'| ,__/''\__,_)(____/(____/
( )_) |       | |
 \___/'       (_)

🌟 Welcome to gopass!
ΓÜá No existing configuration found.
Γÿ¥ Please run 'gopass setup'

Error: not initialized
PS C:\Users\User\Desktop>

@stephengillie
Copy link
Collaborator

I still don't understand why a x64 program requiring a x86 dependency is flagged as a problem? That's the basic issue here, correct?

Yes. I'm guessing matching might have been added to prevent x64 dependencies from being required on 32-bit OSes, since our x86 validation uses a 32-bit OS.

I believe all dependencies have to have the same architecture, installer type, and scope.

If that is really the case, I don't see how winget dependency management is going to work in practice. (NB I understand that a x86-only program requiring a x64-only dependency or a user-scope only program requiring a machine-scope-only dependency needs to block validation, but other than that I don't understand why it would need be that strict).

I understand your concerns and wish I had a better way to address them. It might not be matching on all 3 attributes.

Relatedly, I don't understand, why I as a submitter, can only be alerted to this ☝🏻 after you run your custom/manual validation?

The information might have been in the validation run logs, but I wasn't expecting this error here, so I haven't added it to my log gathering script. The Validation-Executable-Error label is common for CLI programs, so I went to Manual Validation instead of digging deeper into the run logs. Please feel free to gather and review the run logs directly for this and other possible error data.

What CI checks are you running?

I am referring to the validation pipeline here on GH. grafik Why is it necessary for you to have your own additional validation script? And if it is really necessary to have these additional checks, why are they not part the CI pipeline? As a consequence, submitting a manifest drags on for days.

Oh, sorry for misunderstanding.

Unfortunately, not every program can pass Automated Validation, and so we added a very small team to perform a manual review and complete the validation. The manual process can only perform some of the checks that the Automated Validation pipeline performs, and we've pushed the manual process innovations upstream to the Automated pipeline in recent months.

[Sandbox doesn't] support Defender so can't complete the Validation process

x64 vs x86 isn't a Defender problem, though, is it?

No, it's not, as Defender doesn't run in either 32-bit nor 64-bit Sandbox. I wish it did, as it could streamline validation in situations like this.

@wingetbot wingetbot added Azure-Pipeline-Passed Validation pipeline passed. There may still be manual validation requirements. Validation-Completed Validation passed labels Sep 8, 2023
@microsoft-github-policy-service
Copy link
Contributor

dpprdan,

The check-in policies require a moderator to approve PRs from the community.

Our moderators are community volunteers, please be patient and allow them sufficient time to review your submission.

Template: msftbot/requiresApproval/moderator

@microsoft-github-policy-service microsoft-github-policy-service bot added the Moderator-Approved One of the Moderators has reviewed and approved this PR label Sep 8, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot merged commit b2f2c1d into microsoft:master Sep 8, 2023
7 checks passed
@microsoft-github-policy-service
Copy link
Contributor

Hello dpprdan,
Validation has completed.

Template: msftbot/validationCompleted

@wingetbot
Copy link
Collaborator

Publish pipeline succeeded for this Pull Request. Once you refresh your index, this change should be present.

@denelon
Copy link
Contributor

denelon commented Sep 9, 2023

@dpprdan we haven't implemented:

That's part of the issue with dependencies using different architectures.

We won't be fully automating validation with dependencies until a couple of weeks after WinGet 1.6 goes GA (Generally Available). We're targeting the end of September for the go live date, and it will likely be mid-October before automated validation can run validation on manifests with dependencies.

@dpprdan
Copy link
Contributor Author

dpprdan commented Sep 11, 2023

@denelon I still fail to see why a x64 program manifest requiring a x86 dependency is currently flagged as a problem?

(AFAICT #1665 is about manifests requiring a specific dependency architecture (and I assume that the main manifest is x86-only?!), where the dependency is available on more than one architecture. So this doesn't apply here. There is only one dependency architecture available, namely x86 - which runs perfectly fine with the main x64 program.)

@dpprdan dpprdan deleted the gopass.gopass-1.15.7-156752d3-2ee6-4c09-ab43-094b04e4e834 branch September 11, 2023 07:45
@denelon
Copy link
Contributor

denelon commented Sep 15, 2023

We've encountered some interesting scenarios for installers that are x86 (for compatibility reasons) that install x64 versions of software, and which architectures should be used for dependencies. This is slightly compounded by cases where an emulated version of a dependency is running on different hardware. WinGet attempts to pick the best version of a package to install. Sometimes WinGet may not pick the "best" choice.

We don't test every permutation for which installer and dependency combination might get selected. "It works on my machine™️" can cause problems.

I was just trying to loop some of the related topics together. The challenges may not apply to this package, but I know they do for some other packages. We will start wiring up the dependency validation logic in production after the WinGet 1.6 client goes out stable. There will be plenty of older PRs that will get triggered, and hopefully, most of them will pass validation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Matching Azure-Pipeline-Passed Validation pipeline passed. There may still be manual validation requirements. Moderator-Approved One of the Moderators has reviewed and approved this PR Publish-Pipeline-Succeeded Validation-Completed Validation passed Waived-Validation-Executable-Error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants