-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Conversation
Thanks for your contribution! Could you elaborate how you compile? We run amalgamation in our CI and this should have been caught |
I've just tried to compile amalgamation in Ubuntu and everything seems OK. |
Ah! @lebeg wdyt? |
Also fixed #8850. |
@@ -143,7 +143,12 @@ def expand(x, pending, stage): | |||
continue | |||
path = m.groups()[0] | |||
h = path.strip('./') if "../3rdparty/" not in path else path | |||
source = find_source(h, x, stage) | |||
if h.endswith('complex.h') and x.endswith('openblas_config.h'): |
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.
Could you add a comment explaining the special case? Also, is there maybe a nicer way to deal with this ambiguity - e.g. making the names (complex.h) more unique in the output to avoid a name clash?
source = find_source(h, x, stage) | ||
if h.endswith('complex.h') and x.endswith('openblas_config.h'): | ||
source = '' | ||
elif h.startswith('ps/'): |
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 the "ps/" come from? Maybe we can edit where this comes from instead of adding an exception?
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.
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.
Feel free to rewrite that instead. It doesn't seem correct to me.
Could you paste the error log you get on os x into an issue so we have it documented? |
Error log:
|
CachedOp: cc @reminisce @zheng-da |
ping @reminisce @zheng-da could you take a look at this PR? thanks! |
@ufoym could you please add "Fixes #8850" in your PR description? This will automatically close the associated issue on PR merge @mxnet-label-bot add [C++ ] |
@ufoym could you please add "Fixes #8850" in your PR description |
adding @piiswrong for review too |
@ufoym Thanks for the contribution, could you trigger CI again? (you can do that by push an empty commit) |
@roywei Done. |
I have retriggered the CI as it was failed. |
@ufoym please rebase your PR and resolve the merge conflicts |
@anirudhacharya The merge conflicts have been resolved again. |
@anirudhacharya - Ping for review. Thanks! |
@ufoym Could you re-trigger CI again? |
@ufoym Could you please fix build errors? @zheng-da @reminisce Could you please help review the PR? |
@ankkhedia @vandanavk @zheng-da I merely changed the code (just simply resolved the merge conflicts). All checks have passed for several times. Did I miss something? |
* Fix broken amalgamation * Fixes apache#8850 * Update CONTRIBUTORS.md * Update CONTRIBUTORS.md
* Fix broken amalgamation * Fixes apache#8850 * Update CONTRIBUTORS.md * Update CONTRIBUTORS.md
* Fix broken amalgamation * Fixes apache#8850 * Update CONTRIBUTORS.md * Update CONTRIBUTORS.md
* Fix broken amalgamation * Fixes apache#8850 * Update CONTRIBUTORS.md * Update CONTRIBUTORS.md
* Fix broken amalgamation * Fixes apache#8850 * Update CONTRIBUTORS.md * Update CONTRIBUTORS.md
Description
Fixes #8850.
Recent updates introduced new functions whose declarations were expanded by the amalgamation script, but left out the definition because the expansion list did not include all necessary header files.
The fix for now is to
complex.h
.so that amalgamation can be compiled without errors.
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
complex.h
.