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

x86 support #115

Open
wants to merge 30 commits into
base: master
Choose a base branch
from
Open

x86 support #115

wants to merge 30 commits into from

Conversation

iamwacko
Copy link

@iamwacko iamwacko commented Apr 27, 2023

This PR enables x86 and adds x86 testing in CI. Currently on Linux is supported as a host. I'm not to partial to Windows. #114

@dvc94ch
Copy link
Contributor

dvc94ch commented Apr 27, 2023

Can't you merge it with the existing tasks?

@MarijnS95 MarijnS95 linked an issue Apr 27, 2023 that may be closed by this pull request
@dvc94ch
Copy link
Contributor

dvc94ch commented Apr 27, 2023

the ci is broken, once it's green, lgtm

This should fix the broken CI
@dvc94ch
Copy link
Contributor

dvc94ch commented Apr 27, 2023

more typos. I think you can open a PR against your own master so you don't have to wait for me to approve...

uses: hecrj/setup-rust-action@v1
with:
rust-version: stable
targets: "i686-unknown-linux-gnu"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't be necessary. xbuild automatically downloads the required targets using rustup

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it is supposed to, but

error[E0463]: can't find crate for `core`
  |
  = note: the `i686-unknown-linux-gnu` target may not be installed
  = help: consider downloading the target with `rustup target add i686-unknown-linux-gnu`

says otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then maybe something is missing somewhere...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This used to be hardcoded but should be fixed now since #139. @iamwacko can you rebase, remove this setup from the CI (because xbuild must do it to provide a more-or-less out-of-the-box experience), and retest?

@dvc94ch
Copy link
Contributor

dvc94ch commented May 2, 2023

who still uses 32bit anymore? all new devices intended to run a desktop or mobile app are 64bit and have been for years now. outside of embedded systems 32bit is pretty much gone.

@wiredmatt
Copy link

wiredmatt commented Oct 16, 2023

who still uses 32bit anymore? all new devices intended to run a desktop or mobile app are 64bit and have been for years now. outside of embedded systems 32bit is pretty much gone.

I have a Xiaomi phone, brand known for scamming customers by selling them phones with this kind of setup

See this and this.

My device uses armeabi-v7a, and I'm unable to build using either xbuild or cargo-apk without 32bit support. According to those people with actual games released, armeabi-v7a seems to be the most popular architecture in customer devices.

Most people who play mobile games don't live in first world countries, that's the deal. Probably most are from Asia / LATAM / somewhere else, so replying to your question, probably half the world population is still using 32bit / 32bit based architecture devices.

And almost forgot, this as well.

Most Android phones have ARM chipsets. However, many ChromeOS devices use x86 chips. The difference is not important for basic apps written in Kotlin or Java. However, for apps written in native code, including those created with game engines, the chipset in the device can be an important concern.
Ideally, all apps and games with native code ship with all four major Android ABIs (Application Binary Interfaces): armeabi-v7a (arm32), arm64-v8a (arm64), x86 (x86_32), and x86_64. This provides the best performance and lowest battery consumption for each device. For example, a cmake-based build.gradle file might contain...

@p0ryae
Copy link

p0ryae commented Nov 9, 2023

I completely agree with @wiredmatt.

who still uses 32bit anymore? all new devices intended to run a desktop or mobile app are 64bit and have been for years now. outside of embedded systems 32bit is pretty much gone.

That's a pretty bold statement. The project README says the following:

The goal of xbuild is making native app development as easy as web development.

Why adding more support a bad practice in this case? Saying that "who still uses 32bit anymore" unless there's a reasonable program design explanation is fairly bias and impractical. Adding 32-bit support will increase the project's usability and make it reach to a broader audience, which is a positive development, not a negative one.

Comment on lines +323 to +326
// 32 bit binaries can run on 64 bit hardware, of particular use for testing
// This way 32 bit xbuild will produce 32 bit binaries, unless commanded otherwise
if cfg!(target_arch = "x86") {
Ok(false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I follow. This doesn't seem correct as you might be breaking fully-native builds here? That's where xbuild should return true for this function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the source of this file?

run: ./x new template

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please trim (don't add) unnecessary whitespace.

Copy link
Member

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly seeing lots of unnecessary bits in in ci.yml, the rest is mostly uncommenting exisisting x86 codepaths which looks good :)

- uses: actions/upload-artifact@v3
with:
name: i686-x
path: bin/x
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be nice to add the .exe suffix for Windows... And add a 32-bit Windows build?


- name: install x
if: ${{ matrix.host-arch == 'x64'}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if: ${{ matrix.host-arch == 'x64'}}
if: ${{ matrix.host-arch == 'x64' }}

uses: actions/download-artifact@v3
with:
name: ${{ matrix.host }}-x

- name: install x i686
if: ${{ matrix.host-arch == 'x86'}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if: ${{ matrix.host-arch == 'x86'}}
if: ${{ matrix.host-arch == 'x86' }}

steps:
- name: install rust
if: ${{ matrix.target.arch == 'x64'}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if: ${{ matrix.target.arch == 'x64'}}

Comment on lines +91 to +97

- name: install rust i686
if: ${{ matrix.target.arch == 'x86' }}
uses: hecrj/setup-rust-action@v1
with:
rust-version: stable
targets: "i686-unknown-linux-gnu"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- name: install rust i686
if: ${{ matrix.target.arch == 'x86' }}
uses: hecrj/setup-rust-action@v1
with:
rust-version: stable
targets: "i686-unknown-linux-gnu"

- run: chmod +x ./x
if: ${{ matrix.host == 'ubuntu-latest' || matrix.host == 'macos-latest' }}

- name: install llvm ubuntu
if: ${{ matrix.host == 'ubuntu-latest' }}
if: ${{ matrix.host == 'ubuntu-latest'}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert all unnecessary whitespace changes:

Suggested change
if: ${{ matrix.host == 'ubuntu-latest'}}
if: ${{ matrix.host == 'ubuntu-latest' }}

@@ -87,18 +128,31 @@ jobs:
java-version: 11

- name: install linux dependencies
if: ${{ matrix.host == 'ubuntu-latest' }}
if: ${{ matrix.host == 'ubuntu-latest'}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if: ${{ matrix.host == 'ubuntu-latest'}}
if: ${{ matrix.host == 'ubuntu-latest' }}

run: sudo apt-get update && sudo apt-get install libwebkit2gtk-4.0-dev libgtk-3-dev squashfs-tools

- name: install dependencies for i686
if: ${{ matrix.host == 'ubuntu-latest'}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if: ${{ matrix.host == 'ubuntu-latest'}}
if: ${{ matrix.host == 'ubuntu-latest' }}

- name: create project
if: ${{ !((matrix.host == 'macos-latest' || matrix.host == 'windows-latest' ) && matrix.host-arch == 'x86')}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if: ${{ !((matrix.host == 'macos-latest' || matrix.host == 'windows-latest' ) && matrix.host-arch == 'x86')}}
if: ${{ !((matrix.host == 'macos-latest' || matrix.host == 'windows-latest') && matrix.host-arch == 'x86') }}

Comment on lines 145 to +154
- name: build project
if: >
!(matrix.host == 'macos-latest' && matrix.target.platform == 'linux' ||
matrix.host == 'windows-latest' && contains(fromJson('["linux", "macos", "ios"]'), matrix.target.platform))
${{
!(matrix.host-arch == 'x64' && matrix.target.arch == 'x86' ||
matrix.host == 'macos-latest' && matrix.host-arch == 'x86' ||
matrix.host == 'windows-latest' && matrix.host-arch == 'x86' ||
matrix.host == 'macos-latest' && matrix.target.platform == 'linux' ||
matrix.host == 'windows-latest' && contains(fromJson('["linux", "macos", "ios"]'), matrix.target.platform)
)
}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This thing is getting out of hand, perhaps we can create a separate job: for it with a matrix that uses exclude/include instead.

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.

32 bit support
5 participants