feat(install): Add support for x64 microarchitectures #5674
+184
−62
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Everything seems to work as intended now, ready to be merged |
brian6932
force-pushed
the
64bit-v2-v3-v4
branch
from
9322ef9
to
38b0c8d
Compare
|
I don't think so many microarchitectures are needed. Just add specific manifests in |
|
I don't see why it's a bad thing to support this, with that logic there's no point in having a native arm64 build, because Windows can just translate x64 instructions. The introduction of x64 microarchs reduces:
It's not as if Scoop would be doing something out of the ordinary here, many package managers support such features, and not having them will only make package compatibility worse with time. We're on the 4th revision of the x64 architecture, and it's not like instruction sets are backwards compatible. I think proper support here is very important, especially for the future when something like #340 is added and worked on, this will be basically a necessity. |
|
That's another story about How many apps support such microarchitectures now? If there're only one or two (less than ten?), then alternative versions should be used. If there're plenty of apps, of course we should put all architectures in one manifest just like this PR. Correct me if I'm wrong, and other maintainers may have different opinions. |
It's not exactly easy to say, since they'd just be grouped up with x64 at the moment, and may be differentiated in name by many instruction sets, or just not at all. ss regex searchCurrently in my personal scoop bucket, I have 3. And like I said before, this will be a very necessary feature when/if building+compiling functionality is introduced into Scoop. Package managers like Arch Linux's pacman, nix, and a few others have such features. Imo it should be Scoop's ideal for when a user wants to install a package, they get the most optimized one for their system, it's not as if they'd manually have to choose the microarchitecture in this PR, it's automated and decided for them.
I only mentioned it because I saw a good performance improvement from the changes I made. |
I personally don't like the implementation. I do agree that there should be a better support of multiarch, however, simply putting more architecture fields into the manfiest spec is just a rough try imho. Mixing architecture-specific into one single manifest file has been identified as problematic. One obvious issue is as upstream dropping support for specific architectures, or binaries of one of architectures are missing, the autoupdate stops to work. Manual fix PRs for this kind of issue took up a lot of time in the past and I believe other maintainers empathize with it. Old versions of 32bit software became completely unavailable after "fixing" the autoupdate by removing 32bit-specific from the manifest. I've proposed separated structure for further support for architecture-specific manifesting last year (more comments about support of architecture can be found in that PR). It was criticized because of information redundancy, which is a thing I can't disagree with. But redundancy is not that bad in some cases, aforementioned issues, including version conflict issue of different architectures for the same package, could be resolved immediately with the proposed structure. Though architecture detection tweaks are still required, new architecture support will be just a usual thing by creating new subdir for new architecture. Spliting manifest into architecture subdirs is similar to the conventional solution that can be found in other package managers in a way. |
I have to agree with this. Having too many archs in the manifest is additional maintenance burden. I'll be ready to change my mind once the proliferation of x64 microarchs is large enough to affect many apps. As of now, this is rare. |
brian6932
force-pushed
the
64bit-v2-v3-v4
branch
from
892832f
to
a269210
Compare
|
The problem I mentioned can be worse when combined with versioned install. #5582 (comment) |
|
@chawyehsu These microarchitectures have fallback, without any conditions |
|
I see you try to convince me, but fallback does not solve any issue I mentioned. |
|
There's no condition on the fallback, the |
|
Understood, the issue I linked is an example for situation like the one I mentioned - Mixing architecture-specific into one single manifest file has been identified as problematic. - and is common. |
|
We can simply allow any name to be added to the architecture, with the exception that architectures other than 32bit, 64bit, and arm64 will not be recognized in Scoop Core. Users can only install them by |
brian6932
force-pushed
the
64bit-v2-v3-v4
branch
from
6f4125b
to
fc8b622
Compare
brian6932
force-pushed
the
64bit-v2-v3-v4
branch
2 times, most recently
from
521a8c7
to
a4f9a84
Compare
brian6932
force-pushed
the
64bit-v2-v3-v4
branch
2 times, most recently
from
975401b
to
6b9d82b
Compare
brian6932
force-pushed
the
64bit-v2-v3-v4
branch
3 times, most recently
from
af2aa36
to
d22b20e
Compare
brian6932
force-pushed
the
64bit-v2-v3-v4
branch
from
d22b20e
to
e901fd8
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Added support for
x86-x64-v2,x86-x64-v3,x86-x64-v4Also refactored incorrect use of Arrays, to ArrayLists and hashsets, to reduce copies made in the process, and reduce algorithmic complexity
Motivation and Context
Would be nice to be able to support more optimized builds for modern architectures
Relates to ScoopInstaller/Extras#11123 (comment)
How Has This Been Tested?
https://github.com/brian6932/Extras/blob/mpv-git-v3-aio/bucket/mpv-git.json
checkver
install
install -a '64bit-v3'
(fallback test) install -a '64bit-v4'
(fallback test) install -a '64bit-v2'
Checklist:
developbranch.