Skip to content

Handle x86_64 os.arch for native libraries#107289

Merged
rjernst merged 3 commits intoelastic:mainfrom
rjernst:native/x86
Apr 10, 2024
Merged

Handle x86_64 os.arch for native libraries#107289
rjernst merged 3 commits intoelastic:mainfrom
rjernst:native/x86

Conversation

@rjernst
Copy link
Copy Markdown
Member

@rjernst rjernst commented Apr 9, 2024

On some systems Java appears to return amd64 (even if not an amd processor), but on others it returns x86_64. This commit handles the latter case to correctly associate the arch with the appropriate platform dir.

On some systems Java appears to return amd64 (even if not an amd
processor), but on others it returns x86_64. This commit handles the
latter case to correctly associate the arch with the appropriate
platform dir.
@rjernst rjernst added >non-issue :Core/Infra/Core Core issues without another label labels Apr 9, 2024
@rjernst rjernst requested a review from a team as a code owner April 9, 2024 22:08
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Apr 9, 2024
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

Copy link
Copy Markdown
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

This looks good to me. I wonder if we're missing other ones now, not sure how to check this.

Copy link
Copy Markdown
Contributor

@ldematte ldematte left a comment

Choose a reason for hiding this comment

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

LGTM
Self reminder: we'll need something similar when we integrate x64 to native vec (after #106133)

@rjernst
Copy link
Copy Markdown
Member Author

rjernst commented Apr 10, 2024

we'll need something similar when we integrate x64 to native vec

I don't think that is true. The vec code relies on NativeAccess to load the vec library. While vec uses multi release jars, it should not need to determine anything about the architecture, the return value from NativeAccess determines whether vec can be supported. As far as actually loading the library within NativeAccess, once x64 is supported, the conditionals on loading based on arch can be removed since both aarch64 and x64 will be supported.

@ldematte
Copy link
Copy Markdown
Contributor

I don't think that is true. The vec code relies on NativeAccess to load the vec library. While vec uses multi release jars, it should not need to determine anything about the architecture, the return value from NativeAccess determines whether vec can be supported. As far as actually loading the library within NativeAccess, once x64 is supported, the conditionals on loading based on arch can be removed since both aarch64 and x64 will be supported.

That's correct; we will need something similar, but in the gradle script(s) that put the generated libraries in the correct places/upload the artifacts, not in the actual Java code. I'm not sure about removing the conditionals at load time: what if someone runs ES in some "fancy" environment scratch that, there is no Solaris or RISC-V java download, so I guess it's only ARM and x64.

String archname = sysprops.get("os.arch");
String arch;
if (archname.equals("amd64")) {
if (archname.equals("amd64") || archname.equals("x86_64")) {
Copy link
Copy Markdown
Contributor

@ChrisHegarty ChrisHegarty Apr 10, 2024

Choose a reason for hiding this comment

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

I think that this should be fine, and cover the archs we run on. No need for archs such as, say, "i386", "x86", "ppc64", "ppc64le", "aarch64", "riscv64". For reference, some JDK archs here https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/util/Architecture.java

@rjernst rjernst merged commit f5a7d25 into elastic:main Apr 10, 2024
@rjernst rjernst deleted the native/x86 branch April 10, 2024 21:51
craigtaverner pushed a commit to craigtaverner/elasticsearch that referenced this pull request Apr 11, 2024
On some systems Java appears to return amd64 (even if not an amd
processor), but on others it returns x86_64. This commit handles the
latter case to correctly associate the arch with the appropriate
platform dir.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Core/Infra/Core Core issues without another label >non-issue Team:Core/Infra Meta label for core/infra team v8.14.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants