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

Make unspecified 'Arch' to be listed for 32-bit and 64-bit architecture #2684

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Zylquinal
Copy link
Contributor

Description

Make unspecified 'Arch' to be listed for 32-bit and 64-bit architecture. As we know that nearly all of the dependencies listed on the repository doesn't have 'Arch' attribute, and with the missing of these attributes, nearly all of the dependencies listed on the repo can't be seen on the 32-bit bottle environment. It happen due to the way the code return 'Win64' on null or missing attribute.

Fixes #2166

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.
Provide instructions so we can reproduce.

  • Local Machine

image of installed dependencies on 32-bit environment
image of installed program using the modified bottle

@Kinsteen
Copy link
Contributor

I'm afraid this will cause problems when installing some dependencies, as they were made for 64 bits... For example, vcredist2015 installs both x86 and x64 version ; does it crash? Does it silently fail?

@Zylquinal
Copy link
Contributor Author

That's indeed true, but i guess it should be done at repository level. Instead of making a fallback to win64. i think all dependencies must have Arch attribute. Making it clearer for people to know which dependency that actually support the architecture.

@Zylquinal
Copy link
Contributor Author

Since some dependencies could support both architectures, i think the dependency could look like this instead.

Arch: [win32, win64]
Steps: 
- action: install_exe
  arch:
    win32:
      file_name: VC_redist.x86.exe
      url: https://download.microsoft.com/download/9/3/F/93FCF1E7-E6A4-478B-96E7-D4B285925B00/vc_redist.x86.exe
      rename: vcredist2015_x86.exe
      file_checksum: 1a15e6606bac9647e7ad3caa543377cf
      file_size: 13767776
      arguments: /quiet /norestart
    win64:
      file_name: VC_redist.x64.exe
      url: https://download.microsoft.com/download/9/3/F/93FCF1E7-E6A4-478B-96E7-D4B285925B00/vc_redist.x64.exe
      rename: vcredist2015_x64.exe
      file_checksum: 27b141aacc2777a82bb3fa9f6e5e5c1c
      file_size: 14572000
      arguments: /quiet /norestart

@Kinsteen
Copy link
Contributor

Kinsteen commented Feb 17, 2023

I think it would be best to have an option for steps like:

for:
  - "x86"

that you can specify for each step. It could be an array, assumed per default to be ["x86", "x64"], so that we can specify the architecture for each step.

Most of 32 bits stuff also needs to be installed for 64 bits bottles, and it would require less changes on Bottles/dependencies side. The solution you proposed seemed a bit too much imo.

This could be in addition to a global "Arch" property as well

@Zylquinal
Copy link
Contributor Author

Well i guess that's true that it is indeed too much, especially when deciding on updating the repo structure. But what about this.

image 1
image 2

Instead of changing the check, user could press the button to show all of the dependencies, while slowly working on which step should be added on x86. Since i guess it would take quite a lot of time testing each lib and updating each repo step. And anyway i new to this project, never thought it would be like this when trying to fix the custom environment not showing any lib on my Bottles.

@Kinsteen
Copy link
Contributor

This "bug" is there since a long time because there's no easy fix unfortunately...

@Zylquinal
Copy link
Contributor Author

Zylquinal commented Feb 17, 2023

Just changed a few thing here, with the config example. The program will retain its compatibility with older version, and it should be fine for older version to safely read the config. On the Arch, i decide to make it like win64, win32, win32_win64 (or reversed). It also added a button on the dependency menu, so people could choose to view every dependencies without any architecture filter. What do you think?

Example of it installing x86 vcredist2015 on 32-bit config

@Kinsteen
Copy link
Contributor

I would much prefer to have manifests like this:

Name: vcredist2015
Description: Microsoft Visual C++ Redistributable for Visual Studio 2015
Provider: Microsoft
License: Microsoft EULA
License_url: https://www.microsoft.com/web/webpi/eula/net_library_eula_enu.htm
Dependencies: []
Steps:
- action: install_exe
  file_name: VC_redist.x86.exe
  url: https://download.microsoft.com/download/9/3/F/93FCF1E7-E6A4-478B-96E7-D4B285925B00/vc_redist.x86.exe
  rename: vcredist2015_x86.exe
  file_checksum: 1a15e6606bac9647e7ad3caa543377cf
  file_size: 13767776
  arguments: /quiet /norestart

- action: install_exe
  file_name: VC_redist.x64.exe
  url: https://download.microsoft.com/download/9/3/F/93FCF1E7-E6A4-478B-96E7-D4B285925B00/vc_redist.x64.exe
  rename: vcredist2015_x64.exe
  file_checksum: 27b141aacc2777a82bb3fa9f6e5e5c1c
  file_size: 14572000
  arguments: /quiet /norestart
  for:
    - win64
  
- action: override_dll
  dll: api-ms-win-crt-private-l1-1-0
  type: native,builtin

The less changes the better, and even in the Python code it would not require much changes. We assume that by default, each dependency is 32 and 64 bits compatible, and same for each step, unless there is a for clause like in the thing I sent. I think it's better, but I would also like thoughts from other devs

@Zylquinal
Copy link
Contributor Author

Gosh just realized what you mean, that's indeed better XD. Guess thing could go bad when you haven't have any sleep for 24+ hours. Just changed it like what you're suggesting.

This is the example manifest, and this is how it successfully run on 64-bit environment. It also will not affect any older version of the manifest, example of installing dependency with older version of the manifest. I also change it to return win64_win32 when there's no Arch detected on the manifest, and also making the one without for as the global action.

@TheEvilSkeleton
Copy link
Contributor

I'm marking as draft, as it's still under discussion.

@TheEvilSkeleton TheEvilSkeleton marked this pull request as draft February 18, 2023 03:30
…ttribute on the steps action, for multi-architecture support

- Added 'show all dependencies' and 'show allowed dependency' on the dependency menu.
@TheEvilSkeleton TheEvilSkeleton changed the title make unspecified 'Arch' to be listed for 32-bit and 64-bit architecture, fix for #2166 Make unspecified 'Arch' to be listed for 32-bit and 64-bit architecture Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: On New Custom 32bit Bottle - it doesn't list out the Dependencies.
3 participants