ffi: port semi-colon fix for riscv64 (and others)#63794
Conversation
Signed-off-by: Stewart X Addison <sxa@ibm.com>
|
Review requested:
|
|
@sxa I'm not sure about this. I guess we should wait for a official libffi fix and then run the update, or am I wrong? |
Yeah I understand that although I'm not sure when that might be. The last release (the one we're on) was in August last year. Given that it causes a build break at the moment with libffi enabled (albeit currently on an unofficial platform) I figured it would be preferable to float the patch for now and then pick up the fix officially when there is another release. It will be included in 3.6.0 but it currently doesn't have a target release date. (Maybe @atgreen has that information as I don't think there's a regular release schedule) |
|
Re-running |
mcollina
left a comment
There was a problem hiding this comment.
lgtm
I think we should follow the same pattern we have for floating patches on dependencies. I'm not overly concerned because this is still experimental and libffi is not a fast moving target.
Ok, then it LGTM as well. |
|
While it's reasonably clear that it's not related to this PR and therefore does not affect merging I did notice significant flakiness on |
This PR pulls across the fix which has been accepted upstream at libffi/libffi#968 and will therefore be in any future versions of libffi that we take.
@ShogunPanda I'm not sure if we have implemented any specific rules/restrictions on taking fixes like this so hopefully this is ok to go in as a patch since I had it merged upstream before raising this.
Fixes nodejs/unofficial-builds#235 which has the explanation of when and why it occurs.
While this includes all platforms which would have the same issue I'll note that loongarch does not show the problem in the unofficial builds on that platform but that is likely because it is using
gccand nodeclang. Also noting that we haveloongarch64in this PR as opposed to the upstreamloongarchdirectory - that is because of this upstream change where the directory was renamed.FYI @nodejs/platform-loong64 @nodejs/platform-riscv64