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

ios simulator arm64 and arm64e support #1086

Merged
merged 18 commits into from
Jan 28, 2023

Conversation

swasti29
Copy link
Contributor

Update cmake to consider platform passed as argument for building 1DS for ios simulator m1(arm64 and arm64e) architecture

build-ios.sh Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
build-ios.sh Show resolved Hide resolved
Copy link
Contributor

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

Changes look good to me. But it is important to get the approval from @eduardo-camacho before merging it. As mentioned in the mail discussion:


Please make sure to have tested following scenarios (actually linking library and makes sure it works; lipo checking might not be enough):

  1. Simulator on M1
  2. Simulator on Intel
  3. Physical device on M1
  4. Physical device on Intel

@eduardo-camacho
Copy link
Contributor

Changes look good to me. But it is important to get the approval from @eduardo-camacho before merging it. As mentioned in the mail discussion:

Please make sure to have tested following scenarios (actually linking library and makes sure it works; lipo checking might not be enough):

  1. Simulator on M1
  2. Simulator on Intel
  3. Physical device on M1
  4. Physical device on Intel

Do you have an update on testing scenarios above?

CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@eduardo-camacho eduardo-camacho left a comment

Choose a reason for hiding this comment

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

Left a nit

Copy link
Contributor

@lalitb lalitb left a comment

Choose a reason for hiding this comment

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

Have few comments.

build.sh Outdated Show resolved Hide resolved
build-ios.sh Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@swasti29
Copy link
Contributor Author

Changes look good to me. But it is important to get the approval from @eduardo-camacho before merging it. As mentioned in the mail discussion:

Please make sure to have tested following scenarios (actually linking library and makes sure it works; lipo checking might not be enough):

  1. Simulator on M1
  2. Simulator on Intel
  3. Physical device on M1
  4. Physical device on Intel

I have tested with and without optional deployment target paramter by linking library on Simulator and Physical device with both M1 and Intel

@swasti29
Copy link
Contributor Author

I have tested with and without optional deployment target paramter by linking library on Simulator and Physical device with both M1 and Intel

Have few comments.

Resolved

@swasti29
Copy link
Contributor Author

Changes look good to me. But it is important to get the approval from @eduardo-camacho before merging it. As mentioned in the mail discussion:
Please make sure to have tested following scenarios (actually linking library and makes sure it works; lipo checking might not be enough):

  1. Simulator on M1
  2. Simulator on Intel
  3. Physical device on M1
  4. Physical device on Intel

Do you have an update on testing scenarios above?

I have tested with and without optional deployment target paramter by linking library on Simulator and Physical device with both M1 and Intel.

@lalitb
Copy link
Contributor

lalitb commented Jan 27, 2023

@swasti29 - Changes look good now. You should be able to merge this PR once CI checks are completed ( you may have to rerun the CI check incase of transient failures ).

@eduardo-camacho
Copy link
Contributor

@swasti29 - Changes look good now. You should be able to merge this PR once CI checks are completed ( you may have to rerun the CI check incase of transient failures ).

I approved yesterday night, we are good to go. Thanks!

@swasti29 swasti29 merged commit e8eb28f into main Jan 28, 2023
@swasti29 swasti29 deleted the swagup/iphonesimulator_arm64_support branch January 28, 2023 07:43
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.

5 participants