Skip to content

First stage of cleanup in source tree pollution#2105

Merged
sbc100 merged 1 commit intomasterfrom
cleanup_tree
May 16, 2019
Merged

First stage of cleanup in source tree pollution#2105
sbc100 merged 1 commit intomasterfrom
cleanup_tree

Conversation

@sbc100
Copy link
Copy Markdown
Member

@sbc100 sbc100 commented May 15, 2019

Update build-js.sh to output to out directory. This is district
from the bin directory which is used by the cmake build and may or
may not live in the source tree. The out directory currently always
lives in the source tree.

As a followup change I hope to additionally move all test outout into
this tree.

See #2104

Update build-js.sh to output to `out` directory.  This is district
from the `bin` directory which is used by the cmake build and may or
may not live in the source tree.  The `out` directory currently always
lives in the source tree.

As a followup change I hope to additionally move all test outout into
this tree.

See #2104
@sbc100 sbc100 requested review from dschuff, jgravelle-google and kripken and removed request for dschuff, jgravelle-google and kripken May 15, 2019 18:53
Copy link
Copy Markdown
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Why not keep using bin/ for the js output, even if cmake happens to point to somewhere else?

@sbc100
Copy link
Copy Markdown
Member Author

sbc100 commented May 15, 2019

I was a little confusing because BINARYEN_BIN is in the output directory (i.e. not necessarily under the source checkout at all). But the build-js.sh always writes into the source directory.

I'm using "out" as place where all kind of intermiates can be written, and "bin" as the place where the final binaries live. In a followup I'm trying to have all tests write to the "out" directory too.

Copy link
Copy Markdown
Member

@dschuff dschuff left a comment

Choose a reason for hiding this comment

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

In general I like the idea.

Comment thread .gitignore

# autogenerated during the build
src/passes/WasmIntrinsics.cpp
/src/passes/WasmIntrinsics.cpp
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what is the effect of adding the leading / everywhere?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

IIUC is just makes it a more precise match.

e.g.

*.o

vs

/*.o

The latter only match objects at the top level

@kripken
Copy link
Copy Markdown
Member

kripken commented May 16, 2019

Sounds reasonable. I slightly worry because emcc does look at $BINARYEN_ROOT/bin for wasm-opt, wasm2js, etc. I guess we can update that if necessary?

@sbc100
Copy link
Copy Markdown
Member Author

sbc100 commented May 16, 2019

Those tools still build to "bin" and install to "bin". But for out-of-tree build the "bin" the build to is $BUILDDIR/bin.

i'm not changing to locations of any of the cmake targets.. Only the build-js target

@kripken
Copy link
Copy Markdown
Member

kripken commented May 16, 2019

That should be fine then - I don't think we depend on the location of binaryen.js anymore anywhere.

@sbc100 sbc100 merged commit 64e6807 into master May 16, 2019
@sbc100 sbc100 deleted the cleanup_tree branch May 16, 2019 18:01
sbc100 added a commit that referenced this pull request May 16, 2019
sbc100 added a commit that referenced this pull request May 16, 2019
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