-
Notifications
You must be signed in to change notification settings - Fork 384
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
(zotero) Add 64-bit installer (re-try) #2613
base: master
Are you sure you want to change the base?
Conversation
This adds the new install option for 64-bit Zotero and simplifies the update a bit.
✅ Package verification completed without issues. PR is now pending human review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have any requests for changes that need to happen immediately, but a few observations. This PR feels like it is doing more than is needed to update the package. Which in itself I think would be ok, but would prefer to see commits split up a bit more to ease review.
For instance with the nuspec, I think it would be easier to follow/review if there was a commit that moved the contents around without making any changes, and then a commit that adds tags (which I think is the only thing that's actually changing there).
I realize this is somewhat of a continuation of a previous PR, so I will try to go back to that PR and have a look before continuing this review.
The failing GitHub Action I am as sure as I can be is both unrelated to this PR, and not required for approval of this PR.
|
||
The file 'LICENSE.txt' has been obtained from <https://github.com/zotero/zotero/blob/929288f9811a5026053ae154ca08cc4a9da13c52/COPYING> | ||
The file 'LICENSE.txt' has been obtained from | ||
https://raw.githubusercontent.com/zotero/zotero/refs/heads/main/COPYING |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably link directly to the commit that the text comes from, otherwise it is possible the contents change and then this verification becomes invalid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When trying to compare this file against the original, I am finding it hard to follow what's a change of merely moving lines around, and what's a change to line contents.
This adds the new install option for 64-bit Zotero and simplifies the update a bit. This is essentially the same PR as submitted -- and closed -- previously but the version has been updated and it is hopefully "cleaner".
Description
update.ps1
to grab both 32-bit and 64-bit installer URLs, download the EXEs and replace the information in theVERIFICATION.txt
file.chocolateyinstall.ps1
to install either 32-bit or 64-bit flavor of Zotero.VERIFICATION.txt file
to better match the "standards" already in the repo.chocolateyinstall.ps1
andchocolateyuninstall.ps1
files to work for both bit levels.Motivation and Context
32-bit Zotero works fine on 64-bit systems, but regularly pops up a dialog indicating that it's not the most efficient version for the environment. The upgrade from 32-bit to 64-bit is as simple as a re-install, so fixing this package to have both bit-levels was fairly straightforward.
How Has this Been Tested?
In Windows 11 and the official test environment. Installed the 32-bit-only, v7.0.9 package then upgraded to the local, modified v7.0.11 package from this PR with no trouble. Also installed the proposed package without upgrading. Uninstalls worked too.
Types of changes
Checklist: