(1password) Fix 1Password installation regarding issue 1618 #1733
Conversation
|
❌ Package verification failed, please review the Appveyor Logs and the provided Artifacts before requesting a human reviewer to take a look. |
|
✅ Package verification completed without issues. PR is now pending human review |
michaelmumenthaler
changed the title
|
lgtm. |
|
I've been trying to look at this since this morning (ie. 8 hours ago). |
| @@ -37,3 +37,6 @@ Start-Job -ScriptBlock { param($cache_dir) | |||
| } | |||
| } -ArgumentList ($cache_dir) | |||
| Install-ChocolateyPackage @packageArgs | |||
| if ($env:ChocolateyPackageName -ne "1password4") { | |||
There was a problem hiding this comment.
Can you elaborate as to why this is needed? The package name is 1password so it's never going to be 1password4?
There was a problem hiding this comment.
Because the AppVeyor for some reason tried it with 1password4 and failed (see the first commit in this PR and appveyor logs). Not sure what the problem is there, but i got around it that way and the checks passed with the if statement in place.
There was a problem hiding this comment.
@AdmiringWorm Any thoughts on this?
There was a problem hiding this comment.
“1Password 4” is a previous version of the app that is still maintained separately, because after that it was totally rewritten and significantly changed. I don't understand the context here, but FYI it's entirely possible and legit that a package called 1password4 could exist.
There was a problem hiding this comment.
1password4 and 1password are in the same package (this package here), therefore the install and uninstall scripts need settings for 1password and 1password4 because they have different parameters. The code in question is only needed for 1password and not 1password4. But they both use the same install script. That's why this code is needed for a working installation.
There was a problem hiding this comment.
While it is true that the 1password4 and 1password packages is handled in this same location in the repository here, this was originally due to them both being the same just having the exact same logic back then except for the silent arguments used (which are inserted during the update).
The end results when pushed to the community repository should only include the logic that is necessary for the package that was pushed.
Since these two packages now differ, changes would need to be made to either
- Remove the streams, and make them two separate packages in the repository altogether (common logic could still be shared between helper scripts).
- Have two install/uninstall scripts outside of the tools directory, and during the updated copy the appropriate one to the tools directory so it is used.
So while I understand why you added the if statement here, it would still not be right in terms of the end result being uploaded.
There was a problem hiding this comment.
This is still an issue. I see the issue with this is down to the password4 thing. I've ran line 4. I'm not sure why the two versions are running of the same install file but does anyone know enough about this and choco's systems to make the correction @AdmiringWorm surgested?
|
Other than the question I had the installation and upgrade from an earlier version worked. Uninstallation is still completely broken but that's not the point of this PR. |
There was a problem hiding this comment.
@michaelmumenthaler are you in a position to make the changes necessary that I mentioned in my previous comment?
Basically, that the package installation script shouldn't combine the logic of two different packages but either be separated into two different packages, or have a copy of the necessary scripts and copy the correct one to be used during update.
|
❌ Package verification failed, please review the Appveyor Logs and the provided Artifacts before requesting a human reviewer to take a look. |
|
Dear contributor, As this PR seems to have been inactive for 30 days after changes or additional information |
Description
I added the last line, which will execute the 1Password exe in the local Appdata with the arguments "setup" and "--silent" which will complete the installation and make it available to the user in the start menu and appwiz. This is probably not the prettiest solution but it works.
Motivation and Context
Open issue regarding this fix: #1618
How Has this Been Tested?
The installation was tested on a Windows 10 VM with the latest 1Password binary from the official source.
Screenshot (if appropriate, usually isn't needed):
Types of changes
Checklist: