Skip to content
This repository has been archived by the owner on Jun 28, 2022. It is now read-only.

Arm compatibility #23

Merged
merged 1 commit into from
Feb 11, 2022
Merged

Arm compatibility #23

merged 1 commit into from
Feb 11, 2022

Conversation

r1viollet
Copy link
Contributor

@r1viollet r1viollet commented Feb 11, 2022

What does this PR do?

  • Fix compilation error
    c_char can be several types depending on platform.

  • Handle arm64 triplets package config checks

co-authored with @sanchda

Motivation

Delivering on ARM platforms

Additional Notes

None

How to test the change?

The downstream PR is available to test these changes

- Fix compilation error
- Handle arm triplets package config checks
@@ -195,10 +195,10 @@ impl<'a> TryFrom<Slice<'a, u8>> for &'a str {
}
}

impl<'a> TryFrom<Slice<'a, c_char>> for &'a str {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the error I was facing

error[E0119]: conflicting implementations of trait std::convert::TryFrom<Slice<'_, u8>> for type &str:
--> ddprof-ffi/src/lib.rs:198:1
|
189 | impl<'a> TryFrom<Slice<'a, u8>> for &'a str {
| ------------------------------------------- first implementation here
...
198 | impl<'a> TryFrom<Slice<'a, c_char>> for &'a str {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for &str

Copy link
Collaborator

Choose a reason for hiding this comment

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

You read my mind. I think these two tactical change are sufficient since ARM uses unsigned char.

Copy link
Collaborator

@sanchda sanchda left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -23,15 +23,15 @@ target="$(rustc -vV | awk '/^host:/ { print $2 }')"
# provided. At least on Alpine, libgcc_s may not even exist in the users'
# images, so -static-libgcc is recommended there.
case "$target" in
"x86_64-alpine-linux-musl")
"x86_64-alpine-linux-musl"|"aarch64-alpine-linux-musl")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that Linux is sufficiently general in terms of the x86 and ARM builds that handling both in the same section is appropriate.

@r1viollet r1viollet merged commit 8f84ec3 into main Feb 11, 2022
@r1viollet r1viollet deleted the r1viollet/arm_build branch February 11, 2022 13:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants