Skip to content

Improve dynamic linking browser tests. NFC #14448

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 16, 2021
Merged

Improve dynamic linking browser tests. NFC #14448

merged 1 commit into from
Jun 16, 2021

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Jun 14, 2021

  • Remove unnecessary use of EXPORT_ALL
  • Use C over C++ where there is no actual C++ in use.
  • Use MAIN_MODULE=2 where possible (smaller, faster builds)
  • Re-enable disabled test.

@sbc100 sbc100 requested review from RReverser and kripken June 14, 2021 22:54
@sbc100 sbc100 force-pushed the simplify_browser_dylink_tests branch 3 times, most recently from c4b2f03 to e82025b Compare June 15, 2021 04:54
@sbc100 sbc100 requested a review from aheejin June 16, 2021 00:42
aheejin
aheejin approved these changes Jun 16, 2021
View reviewed changes
Copy link
Member

@aheejin aheejin left a comment

Choose a reason for hiding this comment

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

Why was EXPORT_ALL necessary before and not anymore?

@sbc100
Copy link
Collaborator Author

sbc100 commented Jun 16, 2021

Why was EXPORT_ALL necessary before and not anymore?

Specifying MAIN_MODULE=1 + EXPORT_ALL in redundant since MAIN_MODULE=1 implies EXPORT_ALL:

emscripten/emcc.py

Lines 1603 to 1605 in 74d0c9d

if settings.MAIN_MODULE == 1 or settings.SIDE_MODULE == 1:
settings.LINKABLE = 1
settings.EXPORT_ALL = 1

Looks like this was changed in #10079 so I guess it was needed prior to that.

@sbc100 sbc100 force-pushed the simplify_browser_dylink_tests branch from e82025b to 0096e70 Compare June 16, 2021 15:53
@sbc100
Improve dynamic linking browser tests. NFC
3aeb9f0 
- Remove unnecessary use of `EXPORT_ALL`
- Use C over C++ where there is no actual C++ in use.
- Use `MAIN_MODULE=2` where possible (smaller, faster builds)
- Re-enable disabled test.
@sbc100 sbc100 force-pushed the simplify_browser_dylink_tests branch from 0096e70 to 3aeb9f0 Compare June 16, 2021 15:57
@sbc100 sbc100 merged commit 9ea47e0 into main Jun 16, 2021
@sbc100 sbc100 deleted the simplify_browser_dylink_tests branch June 16, 2021 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aheejin aheejin aheejin approved these changes

@RReverser RReverser Awaiting requested review from RReverser

@kripken kripken Awaiting requested review from kripken

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sbc100 @aheejin