Skip to content

Look for bin/binaryen_js.js directly, instead of expecting us to copy it.#2508

Merged
kripken merged 4 commits intomasterfrom
b_js
Dec 10, 2019
Merged

Look for bin/binaryen_js.js directly, instead of expecting us to copy it.#2508
kripken merged 4 commits intomasterfrom
b_js

Conversation

@kripken
Copy link
Copy Markdown
Member

@kripken kripken commented Dec 6, 2019

Without this, just building and running the test suite didn't work.

@kripken kripken requested a review from sbc100 December 6, 2019 00:09
Comment thread scripts/test/shared.py Outdated
@dcodeIO
Copy link
Copy Markdown
Contributor

dcodeIO commented Dec 6, 2019

Makes me wonder whether file(RENAME ...)ing the file once compilation is complete would be more straight-forward, so everyone building it just gets the file name they'd expect. I assume there is a problem with that?

@kripken
Copy link
Copy Markdown
Member Author

kripken commented Dec 9, 2019

@dcodeIO If that works it sounds good. I don't know much cmake though, and after reading the docs, I'm still not sure how to make that happen after the build? How does it order the operations?

@kripken
Copy link
Copy Markdown
Member Author

kripken commented Dec 10, 2019

Let's merge this in for now, as it improves the status quo, but if we can do even better with a cmake file rename then let's look into that too!

@kripken kripken merged commit 183bfb9 into master Dec 10, 2019
@kripken kripken deleted the b_js branch December 10, 2019 19:05
@dcodeIO
Copy link
Copy Markdown
Contributor

dcodeIO commented Dec 10, 2019

Appears we may instead append

set_target_properties(binaryen_js PROPERTIES OUTPUT_NAME "binaryen")

to get

[ 98%] Linking CXX executable bin/binaryen.js

Documentation says

OUTPUT_NAME sets the real name of a target when it is built and can be used to help create two targets of the same name even though CMake requires unique logical target names.

I don't know much about CMake and whether there may be issues with this, but works for me locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants