Skip to content

Update NDK to 26.0.10792818#17852

Merged
snnn merged 6 commits intomainfrom
snnn/update_ndk
Oct 12, 2023
Merged

Update NDK to 26.0.10792818#17852
snnn merged 6 commits intomainfrom
snnn/update_ndk

Conversation

@snnn
Copy link
Contributor

@snnn snnn commented Oct 9, 2023 •
edited
Loading

Description

Update NDK to 26.0.10792818 which is included in every macOS build machine so that we do not need to download a different version every time in every build.

Motivation and Context

Downloading NDK on-the-fly is a main contributor of Android related build failures.

@snnn snnn requested review from a team and edgchen1 October 9, 2023 22:23
@edgchen1
Copy link
Contributor

edgchen1 commented Oct 9, 2023

should we just update to the latest LTS version, r26?
https://developer.android.com/ndk/downloads#lts-downloads

@snnn snnn marked this pull request as draft October 9, 2023 23:18
@snnn
Copy link
Contributor Author

snnn commented Oct 9, 2023

It seems not easy.

mszhanyi
mszhanyi previously approved these changes Oct 10, 2023
View reviewed changes
edgchen1
edgchen1 reviewed Oct 10, 2023
View reviewed changes
@skottmckay
Copy link
Contributor

skottmckay commented Oct 12, 2023

It seems not easy.

What's not easy? The image includes 26.0.10792818, and the Android SDKs and NDKs appears to be the same versions across all the macos and linux images I looked at.

https://github.com/actions/runner-images/blob/main/images/macos/macos-12-Readme.md

@skottmckay
Copy link
Contributor

skottmckay commented Oct 12, 2023

There are also predefined environment variables for the NDK path that we could maybe use instead of calculating our own NDK_PATH value

https://github.com/actions/runner-images/blob/main/images/macos/macos-12-Readme.md#environment-variables-2

@snnn
Copy link
Contributor Author

snnn commented Oct 12, 2023

New NDK version means new compiler/new linker version. As you can see there are some wired link errors after I made the change. Tomorrow I will try to reproduce them locally

@skottmckay
Copy link
Contributor

skottmckay commented Oct 12, 2023

Yeah it's odd. I have a local WSL2 setup with NDK 25.2 that I used a couple of months ago and it built ORT without a problem back then, and now it's complaining about this being missing (same as the CI):

onnxruntime/include/onnxruntime/core/framework/data_types.h

Line 250 in 74a8acf

std::ostream& operator<<(std::ostream& out, MLDataType data_type);

It's defined in the framework library and that hasn't changed for years. Possibly some other cmake change is affecting the libraries the test exe is building against.

onnxruntime/onnxruntime/core/framework/data_types.cc

Lines 1463 to 1472 in 74a8acf

std::ostream& operator<<(std::ostream& out, const DataTypeImpl* data_type) {
if (data_type == nullptr)
return out << "(null)";
#ifdef ORT_NO_RTTI
return out << "(unknown type)";
#else
return out << typeid(*data_type).name();
#endif
}
)

FWIW building with the 26.0 NDK works. https://dev.azure.com/onnxruntime/onnxruntime/_build/results?buildId=1166226&view=logs&j=b27c03c3-79f9-5527-4b3a-dcfd5ff9627d&t=40e3fc77-3920-5dd0-c3e1-53eae25358ab

As the security guidance is to use the latest NDK maybe we should just try and do that instead of chasing down a link error that occurs with 25.2 but not 25.0 or 26.0 (or any other build of ORT).

@snnn snnn changed the title Update NDK to 25.2.9519653 Update NDK to 26.0.10792818 Oct 12, 2023
@snnn snnn marked this pull request as ready for review October 12, 2023 07:51
@skottmckay
Copy link
Contributor

skottmckay commented Oct 12, 2023

/onnxruntime_src/onnxruntime/test/onnx/TestCase.cc:189:7: error: variable 'item_id' set but not used [-Werror,-Wunused-but-set-variable]
int item_id = 1;

May need to add [[maybe_unused]] to the item_id variable as the usage is in a catch block that gets thrown away in a build with no exceptions.

@snnn snnn requested a review from edgchen1 October 12, 2023 20:05
@snnn snnn merged commit 3f3ece4 into main Oct 12, 2023
@snnn snnn deleted the snnn/update_ndk branch October 12, 2023 21:08
@snnn snnn mentioned this pull request Nov 17, 2023
Merged
jchen351 pushed a commit that referenced this pull request Nov 17, 2023
Update NDK version to 26.1.10909125 (#18493)
41f9379 
### Description
Similar to #17852


### Motivation and Context
To avoid downloading NDK
kleiti pushed a commit to kleiti/onnxruntime that referenced this pull request Mar 22, 2024
@kleiti
Update NDK to 26.0.10792818 (microsoft#17852)
a937811 
### Description
Update NDK to 26.0.10792818 which is included in every macOS build
machine so that we do not need to download a different version every
time in every build.

### Motivation and Context
Downloading NDK on-the-fly is a main contributor of Android related
build failures.
kleiti pushed a commit to kleiti/onnxruntime that referenced this pull request Mar 22, 2024
@kleiti
Update NDK version to 26.1.10909125 (microsoft#18493)
5b94c17 
### Description
Similar to microsoft#17852


### Motivation and Context
To avoid downloading NDK
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@edgchen1 edgchen1 edgchen1 approved these changes

@mszhanyi mszhanyi mszhanyi left review comments

+1 more reviewer

@jchen351 jchen351 jchen351 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@snnn @edgchen1 @skottmckay @mszhanyi @jchen351