-
-
Notifications
You must be signed in to change notification settings - Fork 7
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
release 98 #38
release 98 #38
Conversation
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( I do have some suggestions for making it better though... For recipe:
Documentation on acceptable licenses can be found here. |
…da-forge-pinning 2020.11.08.20.44.44
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.
Thanks Wolf! 😄 Sorry for letting things languish a bit here 😞
Had a few questions about the changes here. Generally they seem ok though. Thanks again for the work here!
url: https://github.com/WebAssembly/binaryen/archive/version_{{ version }}.tar.gz | ||
sha256: 9f805db6735869ab52cde7c0404879c90cf386888c0f587e944737550171c1c4 | ||
patches: | ||
- gcc_stdmacro.patch # [linux] |
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.
Where does this patch come from? Is upstream aware of this?
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 found this fix here (exact same issue: tensorflow/tensorflow#12998 (comment))
Not sure if upstream is aware, I think it's a gcc vs clang thing.
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.
Initially I attempted to build with clang, but this here is much simpler and more in line with cf now.
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.
Sure using gcc
sounds preferable. Just want to make sure this gets fixed upstream. Could you please send a PR to them?
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
I reverted my changes to cmake so that it's closer to what you had. Hope this works now. |
@jakirkham this should be ready. It might be needed for my emscripten PR so would be cool to get this in soon! conda-forge/staged-recipes#13178 If you want to be maintainer on emscripten, let me know! |
${wasm_HEADERS} | ||
) | ||
+# needed for gcc to compile the llvm sources included here | ||
+add_definitions(-D__STDC_FORMAT_MACROS) |
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.
Instead of patching, you can set it in CXXFLAGS
and CFLAGS
env variables
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.
Oops, sorry didn't see this before. Making this change also makes sense.
Thanks Wolf! 😄 |
Checklist
0
(if the version changed)conda-smithy
(Use the phrase@conda-forge-admin, please rerender
in a comment in this PR for automated rerendering)Closes #37