-
Notifications
You must be signed in to change notification settings - Fork 908
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Binaryen116 #3958
Binaryen116 #3958
Conversation
@deadprogram this is the new PR, which updates to binaryen 116 |
Ensure latest version of binaryen is used by GH actions on linux. Required to fix issue#3881 Signed-off-by: Flavio Castelli <fcastelli@suse.com>
Signed-off-by: Flavio Castelli <fcastelli@suse.com>
Signed-off-by: Flavio Castelli <fcastelli@suse.com>
Thank you for the update @flavio I am going to squash/merge now. |
command: git submodule update --init | ||
command: git submodule update --init --recursive |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering, is this --recursive
a new requirement? (Because it also pulls another submodule that we don't actually need).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without that, the code would not compile because of some missing dependency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I found it. The extra dependency will be disabled here: #4154
dependencies: update binaryen submodule to version 116 Signed-off-by: Flavio Castelli <fcastelli@suse.com> Co-authored-by: DarkByteBen <ben@darkbytelabs.com>
It's not generally needed. It was added in #3958 to fix an issue with binaryen that has since been fixed in a different way, so we don't need the googletest dependency anymore.
It's not generally needed. It was added in #3958 to fix an issue with binaryen that has since been fixed in a different way, so we don't need the googletest dependency anymore.
It's not generally needed. It was added in #3958 to fix an issue with binaryen that has since been fixed in a different way, so we don't need the googletest dependency anymore.
This PR is based on #3882 which seems to be no longer active.
I've run into issue #3881 too, this is why I would like to see this fix merged.
I've rebased the original PR against latest
dev
branch, and applied the requests from this comment.Moreover, I've updated to latest release of binaryen v116, instead of v114 of the original PR
I've searched for other places inside of the GitHub actions where the binaryen code is checkout out, but this seems to be specific to Linux. On Mac/Windows the binaryen binaries are installed via package managers (brew and scoop).