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

Fix building with Emscripten toolchain #111

Closed
wants to merge 1 commit into from

Conversation

volo-zyko
Copy link
Contributor

Proposed changes

From the commit message

The fix includes the following:

  • Fix "ambiguity found when searching for best transformation" error by overriding corresponding generator.
  • Change <link-optimization> setting to a proper boolean value. Current Emscripten doesn't support anything else.
  • Add detection of ar and ranlib which are necessary for creating static libraries. BTW, building with shared libraries is possible but it requires additionally setting MAIN_MODULE/SIDE_MODULE flags.
  • Add customization for detection of NodeJS.
  • Set NodeJS as <testing.launcher> which is necessary when during features/environment inspection boost needs to compile and run a test program.
  • Remove DEMANGLE_SUPPORT flag from <debug-symbols> setting which, on its own, is not enough for wasm code generator and is not supported when generating JavaScript.
  • Remove $(FINDLIBS-ST-PFX) and $(FINDLIBS-SA-PFX) from link action as those correspond to -Bstatic and -Bdynamic flags which are not supported by wasm-ld.
  • Add web as a new <target-os> value. It is necessary to disable appending of -pthread compiler flag.

The first change mentioned above was tested here.

Types of changes

What types of changes does your code introduce?

Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Documentation content changes
  • Other (please describe):

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I searched the discussions
  • I searched the closed and open issues
  • I read the contribution guidelines
  • I added myself to the copyright attributions for significant changes
  • I checked that tests pass locally with my changes
  • I added tests that prove my fix is effective or that my feature works
  • I added necessary documentation (if appropriate)

Further comments

The most questionable part of this PR is adding the new web <target-os> value just to disable adding -pthread compiler flag. I thought that <threading>single must have done the job but it haven't.

The fix includes the following:
- Fix "ambiguity found when searching for best transformation"
error by overriding corresponding generator.
- Change <link-optimization> setting to a proper boolean value.
Current Emscripten doesn't support anything else.
- Add detection of ar and ranlib which are necessary for creating
static libraries. BTW, building with shared libraries is possible
but it requires additionally setting MAIN_MODULE/SIDE_MODULE flags.
- Add customization for detection of NodeJS.
- Set NodeJS as <testing.launcher> which is necessary when
during features/environment inspection boost needs to compile and
run a test program.
- Remove DEMANGLE_SUPPORT flag from <debug-symbols> setting which,
on its own, is not enough for wasm code generator and is not
supported when generating JavaScript.
- Remove $(FINDLIBS-ST-PFX) and $(FINDLIBS-SA-PFX) from link
action as those correspond to -Bstatic and -Bdynamic flags which
are not supported by wasm-ld.
- Add web as a new <target-os> value. It is necessary to disable
appending of -pthread compiler flag.
@@ -94,12 +120,13 @@ actions compile.c++

actions archive
{
"$(CONFIG_COMMAND)" $(AROPTIONS) -r -o "$(<)" "$(>)"
"$(.AR)" $(AROPTIONS) rc "$(<)" "$(>)"
"$(.RANLIB)" "$(<)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is ranlib really needed? AFAIK ar s does the same thing for a long time. See #56.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed there is such option and according to Wikipedia it seems the overall processes is geared towards ranlib deprecation. I'll remove it.

toolset.flags emscripten.link OPTIONS <link-optimization>off : --llvm-lto 0 ;
toolset.flags emscripten.link OPTIONS <link-optimization>on : --llvm-lto 1 ;
toolset.flags emscripten.link OPTIONS <link-optimization>full : --llvm-lto 3 ;
toolset.flags emscripten.link OPTIONS <link-optimization>on : -flto ;
Copy link
Contributor

Choose a reason for hiding this comment

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

FIY: link-optimization option is undocumented and presented only in emscripten, there is a documented lto feature that does the same thing. If emscripten like Clang supports -flto=thin/-flto=full you might be better to switch the toolset to inherit from Clang instead of GCC.

Copy link
Contributor Author

@volo-zyko volo-zyko Nov 19, 2021

Choose a reason for hiding this comment

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

there is a documented lto feature that does the same thing

Should link-optimization be renamed to lto? The meaning now is the same.

If emscripten like Clang supports -flto=thin/-flto=full

No. Despite it's based on Clang Emscripten's LTO implementation is a bit more complicated. See its documentation. In other words lto-mode is not applicable here.

In general, renaming to lto seems like a right thing because according to the docs above -flto should be used during compilation as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

No. Despite it's based on Clang Emscripten's LTO implementation is a bit more complicated. See its documentation. In other words lto-mode is not applicable here.

I think emscripten-core/emscripten#10868 tells an opposite thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, the official Emscripten documentation is out of date. I found such custom processing of -flto in emcc.py. Do you think that inheriting from clang (which, BTW, on its own inherits from gcc) should be done in this PR? I tried to inherit from clang but it didn't work out of the box. So, there will be a lot more changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you inherit from clang or clang-linux? You should have been inheriting from clang-linux (that 'linux' suffix means clang with default/posix interface, not the os), sorry I did not tell you that. I would try doing it myself but I don't have the toolset and the CI doesn't tests it either. Don't bother with this if you are short on time.

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 used clang-linux. clang is kind of wrapper around clang-win, clang-linux, etc. I will try to work it out but don't expect too much from me. My knowledge of bjam is rather scarce.

}

toolset.flags emscripten.link USER_OPTIONS <linkflags> ;

actions link bind LIBRARIES
{
"$(CONFIG_COMMAND)" $(USER_OPTIONS) -L"$(LINKPATH)" -o "$(<)" "$(>)" "$(LIBRARIES)" $(START-GROUP) $(FINDLIBS-ST-PFX) -l$(FINDLIBS-ST) $(FINDLIBS-SA-PFX) -l$(FINDLIBS-SA) $(END-GROUP) $(OPTIONS)
"$(CONFIG_COMMAND)" $(USER_OPTIONS) -L"$(LINKPATH)" -o "$(<)" "$(>)" "$(LIBRARIES)" $(START-GROUP) -l$(FINDLIBS-ST) -l$(FINDLIBS-SA) $(END-GROUP) $(OPTIONS)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need to modify GCC instead

b2/src/tools/gcc.jam

Lines 933 to 941 in 371b47a

# On *nix mixing shared libs with static runtime is not a good idea.
toolset.flags gcc.link FINDLIBS-ST-PFX <target-os>$(generic-os)/<runtime-link>shared : -Wl,-Bstatic ;
toolset.flags gcc.link FINDLIBS-SA-PFX <target-os>$(generic-os)/<runtime-link>shared : -Wl,-Bdynamic ;
# On windows allow mixing of static and dynamic libs with static
# runtime is not a good idea.
toolset.flags gcc.link FINDLIBS-ST-PFX <target-os>windows/<runtime-link>static : -Wl,-Bstatic ;
toolset.flags gcc.link FINDLIBS-SA-PFX <target-os>windows/<runtime-link>static : -Wl,-Bdynamic ;
toolset.flags gcc.link OPTIONS <target-os>windows/<runtime-link>static : -Wl,-Bstatic ;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can these settings be overridden in emscripten.jam?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will the following in emscripten.jam do the job?

    toolset.flags emscripten.link FINDLIBS-ST-PFX <target-os>web/<runtime-link>static : ;
    toolset.flags emscripten.link FINDLIBS-SA-PFX <target-os>web/<runtime-link>static : ;

Anyway, I will try.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, it does not override, it appends additional value.

ShuangLiu1992 added a commit to ShuangLiu1992/b2 that referenced this pull request Oct 19, 2022
ShuangLiu1992 added a commit to ShuangLiu1992/b2 that referenced this pull request Oct 19, 2022
ShuangLiu1992 added a commit to ShuangLiu1992/b2 that referenced this pull request Oct 19, 2022
ShuangLiu1992 added a commit to ShuangLiu1992/b2 that referenced this pull request Oct 19, 2022
ShuangLiu1992 added a commit to ShuangLiu1992/b2 that referenced this pull request Oct 19, 2022
ShuangLiu1992 added a commit to ShuangLiu1992/b2 that referenced this pull request Oct 19, 2022
ShuangLiu1992 added a commit to ShuangLiu1992/b2 that referenced this pull request Oct 19, 2022
ShuangLiu1992 added a commit to ShuangLiu1992/b2 that referenced this pull request Oct 19, 2022
ShuangLiu1992 added a commit to ShuangLiu1992/b2 that referenced this pull request Oct 19, 2022
@Kojoley Kojoley mentioned this pull request May 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants