Skip to content
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

Clarify how to compile from source a little more #9383

Merged
merged 2 commits into from
Sep 6, 2019

Conversation

Beuc
Copy link
Contributor

@Beuc Beuc commented Sep 4, 2019

Following-up on the discussion from #9317 :)

Besides a couple trivial fixes, I think we need to explain what version of LLVM and Binaryen needs to be checked-out.

Also, I'm not sure whether there's a bug in tools/ports/binaryen.py since it still references v86, while the emsdk binaries grab v89 AFAIU. If that's intended so probably needs to be clarified, because the port is built and used automatically when BINARYEN_ROOT is empty.

@sbc100
Copy link
Collaborator

sbc100 commented Sep 4, 2019

tools/ports/binaryen.py is just out of date think. I don't think it gets a lot attention of testing/attension because most users are either using emsdk (most end users), or directly working with a binaryen source tree (i.e. core emscripten devs).

Copy link
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.

@kripken
Copy link
Member

kripken commented Sep 4, 2019

Good points about the binaryen port version. It's basically broken as it is, and untested. How about if we just remove it entirely, as planned, now that we have it properly built in the emsdk like llvm?

cmake .. -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_PROJECTS='lld;clang' -DLLVM_TARGETS_TO_BUILD="host;WebAssembly" -DLLVM_INCLUDE_EXAMPLES=OFF -DLLVM_INCLUDE_TESTS=OFF
cd build/
cmake ../llvm -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_PROJECTS='lld;clang' -DLLVM_TARGETS_TO_BUILD="host;WebAssembly" -DLLVM_INCLUDE_EXAMPLES=OFF -DLLVM_INCLUDE_TESTS=OFF
make -j$(nproc) # no need to install
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would replace this with ninja since it does automatic parallelism.

And perhaps replace no need to install with a followup paragraph saying something like "Then setting your LLVM_ROOT in .emscripten to point to <llvm_src>/build/bin"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using ninja complexifies the cmake command, and doesn't seem to check for available memory, so I'll leave this part out (cmake --build .).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe the ninja generator is smart enough to perform compilation with high level of parallelism and one a very low level (1 or 2 jobs in parallel) when does linking. This is something that helps a lot with OOM and large links. . and something I don't think make can do at all. Just one of many reasons to choose ninja. Although you are right its probably out of scope for these instructions.


Please refer to the upstream docs for more detail.

The version to select for e.g. 1.38.43 is --- TODO ---.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you need this. Users building from source normally just want ToT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, in my experience I needed 1 patch (emscripten-core/emscripten-fastcomp#195) for fastcomp but otherwise stuck to identified, reproducible releases. I'll add kripken's link as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess it depends who you are targeting with these instructions. If you want to build a pinned release from source then emsdk is the easiest way to do that. It can download and build everything from source in a single command. I assume these instructions are targetting people who want to work on the sources in place?

However I don't see any harm in documenting that the fastcomp and binaryen are tagged with emscripten releases. Remember upstream llvm has no such tags though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If emsdk can rebuild everything from source, including a patch or two, then that sounds like something to write proeminently here! I had no idea.
Is that working for -upstream yet, I didn't find the right 'emsdk install' tag?

Copy link
Collaborator

Choose a reason for hiding this comment

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

emsdk only knows how to build pinned releases. So you don't get a chance to edit or patch the code in that case.

Thats why I assumed these instructions were more directed at people who want to work on the codebase.

Perhaps we should add link to this doc saying something like: Unless you need to modify of customize llvm or binaryen then emsdk is the easiest way to build them from srouce. See <link>.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only found https://github.com/emscripten-core/emsdk/blob/master/README.md#how-do-i-track-the-latest-emscripten-development-with-the-sdk which is about rebuilding master/incoming for fastcomp, maybe there's a better link.

site/source/docs/building_from_source/index.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

lgtm

@Beuc
Copy link
Contributor Author

Beuc commented Sep 6, 2019

I feel there is an ongoing misunderstanding about what I believe is a common use case for recompiling, so I detailed it at:
https://groups.google.com/forum/#!topic/emscripten-discuss/XoXMcxo9V1U

@kripken
Copy link
Member

kripken commented Sep 6, 2019

lgtm too, thanks!

@kripken kripken merged commit dbe34e0 into emscripten-core:incoming Sep 6, 2019
belraquib pushed a commit to belraquib/emscripten that referenced this pull request Dec 23, 2020
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