Skip to content

Add EXPORT_KEEPALIVE to export symbols even in MINIMAL_RUNTIME #18819

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 6 commits into from
Feb 27, 2023
Merged

Add EXPORT_KEEPALIVE to export symbols even in MINIMAL_RUNTIME #18819

merged 6 commits into from
Feb 27, 2023

Conversation

@DavidPeicho
Copy link
Contributor

@DavidPeicho DavidPeicho commented Feb 22, 2023

This is the continuity of #17911, and an attempt to fix #18814.

@DavidPeicho DavidPeicho mentioned this pull request Feb 22, 2023
Closed
sbc100
sbc100 reviewed Feb 22, 2023
View reviewed changes
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! Can we add some tests though? At least one test for building with -sEXPORT_KEEPALIVE=0 in the normal runtime and one for -sEXPORT_KEEPALIVE= in the minimal runtime?

sbc100
sbc100 reviewed Feb 22, 2023
View reviewed changes
@DavidPeicho
Copy link
Contributor Author

DavidPeicho commented Feb 22, 2023

@sbc100 Done! Fixed the comments and added two tests

@DavidPeicho DavidPeicho force-pushed the feature/export-keepalive branch from 0eac5df to fcb0809 Compare February 22, 2023 18:41
sbc100
sbc100 reviewed Feb 22, 2023
View reviewed changes
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 % comments

@DavidPeicho
Copy link
Contributor Author

DavidPeicho commented Feb 22, 2023 •
edited
Loading

@sbc100 Done! I had to make some gymnastic again for MINIMAL_RUNTIME because there is no export in it. Seems pretty good to me otherwise.

Took the liberty to also update the ChangeLog.

sbc100
sbc100 reviewed Feb 22, 2023
View reviewed changes
@DavidPeicho
Copy link
Contributor Author

DavidPeicho commented Feb 22, 2023

Done :) I think the failing test happens also on master, doesn't look related to this MR.

@sbc100 sbc100 enabled auto-merge (squash) February 23, 2023 01:58
auto-merge was automatically disabled February 23, 2023 10:21

Head branch was pushed to by a user without write access

@DavidPeicho DavidPeicho force-pushed the feature/export-keepalive branch from 45d8735 to dea9657 Compare February 23, 2023 10:21
sbc100
sbc100 reviewed Feb 23, 2023
View reviewed changes
sbc100
sbc100 reviewed Feb 23, 2023
View reviewed changes
sbc100
sbc100 approved these changes Feb 23, 2023
View reviewed changes
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, just a couple of more minor comments.

@DavidPeicho
Copy link
Contributor Author

DavidPeicho commented Feb 23, 2023

@sbc100 Done :)

@DavidPeicho DavidPeicho force-pushed the feature/export-keepalive branch 2 times, most recently from 293b78a to e1c8123 Compare February 23, 2023 19:08
sbc100
sbc100 reviewed Feb 25, 2023
View reviewed changes
@sbc100 sbc100 enabled auto-merge (squash) February 25, 2023 02:04
auto-merge was automatically disabled February 25, 2023 17:08

Head branch was pushed to by a user without write access

@DavidPeicho DavidPeicho force-pushed the feature/export-keepalive branch from e01da91 to cefc557 Compare February 25, 2023 17:08
@DavidPeicho
Copy link
Contributor Author

DavidPeicho commented Feb 27, 2023

@sbc100 The tests were red so I rebased on main. Should be good now

@sbc100 sbc100 merged commit b6d09b0 into emscripten-core:main Feb 27, 2023
impact-maker pushed a commit to impact-maker/emscripten that referenced this pull request Mar 17, 2023
@DavidPeicho @impact-maker
Add EXPORT_KEEPALIVE to export symbols even in MINIMAL_RUNTIME (emscr…
ae8f4f8 
…ipten-core#18819)
impact-maker pushed a commit to impact-maker/emscripten that referenced this pull request Mar 17, 2023
@DavidPeicho @impact-maker
Add EXPORT_KEEPALIVE to export symbols even in MINIMAL_RUNTIME (emscr…
c2be437 
…ipten-core#18819)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sbc100 sbc100 sbc100 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Feature Request: MINIMAL_RUNTIME + MODULARIZE selective export of symbols

2 participants

@DavidPeicho @sbc100