Skip to content

Conversation

derselbst
Copy link
Contributor

I'm currently unable to cross-compile libffi for arm64. The "compile assembly" execute_process() step silently fails and CMake later complains that the .obj file is not available. To avoid hiding errors, the return values of the execute_process() commands should be checked and a fatal error should be issued.

(Here's the build log, in case smb. is interested: https://ci.appveyor.com/project/derselbst/fluidsynth-g2ouw/builds/31479626/job/hm9u92nejl7083dx#L596)

@PhoebeHui
Copy link
Contributor

@derselbst, thanks for bringing this up!

We considered a fix for libffi:arm64-wiindows port or upstream needed instead.

@JackBoosY
Copy link
Contributor

Hi @derselbst, I've fixed this issue in #10485, please update vcpkg and rebuild libffi.

Thanks.

@JackBoosY JackBoosY closed this Apr 7, 2020
@derselbst
Copy link
Contributor Author

It works now, thanks.

But why closing this? This PR only adds additional error checking and should still be merged. Calling execute_process() and blindly hoping that it succeeds is really bad practice.

@JackBoosY JackBoosY reopened this Apr 8, 2020
@JackBoosY JackBoosY closed this Apr 8, 2020
@JackBoosY JackBoosY reopened this Apr 8, 2020
@JackBoosY
Copy link
Contributor

/azp run

@JackBoosY
Copy link
Contributor

@derselbst Sorry, I closed this PR by mistake.

Copy link
Contributor

@JackBoosY JackBoosY left a comment

Choose a reason for hiding this comment

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

Please update libffi version info in VCPKG_PATH/ports/libffi/CONTROL.
See documentation.

@JackBoosY
Copy link
Contributor

/azp run

To avoid hiding errors, the return values of the execute_process()
commands should be checked and a fatal error should be issued.
@msftclas
Copy link

msftclas commented Apr 8, 2020

CLA assistant check
All CLA requirements met.

@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed waiting for response labels Apr 9, 2020
@JackBoosY
Copy link
Contributor

LGTM, thanks for this PR!

@ras0219-msft ras0219-msft merged commit 959a9ea into microsoft:master Apr 16, 2020
@derselbst derselbst deleted the libffi-check-ret-val branch April 17, 2020 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants