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

Add --strip-names option to wasm-split #412

Merged
merged 1 commit into from
Apr 21, 2021
Merged

Conversation

samsartor
Copy link
Contributor

@samsartor samsartor commented Apr 16, 2021

The --strip option of wasm-split leaves symbol names intact. Specifically, it does not remove the "name" custom section
. This may be desired in some cases, but it significantly increases the size of the wasm bundle and potentially leaks source info to users.

This PR adds a --strip-names flag to remove the "name" section as well.

I considered adding a flag (e.g. --strip-by [NAME]) to strip arbitrary custom sections matching NAME (which confusingly, could be "name"), but decided against it. Let me know if that would be preferable.

I haven't yet had the chance to verify that our wasm bundle still works with sentry itself after removing the name section, but I'm guessing it will. I won't merge here until that is tested, but I figured it would be good to get reviewed ASAP.

@samsartor samsartor requested a review from a team April 16, 2021 17:10
@samsartor
Copy link
Contributor Author

I couldn't find a contributing guide for this repo, so I just did my best. It seems like there is some naming convention needed for CI? @flub what should I do to adhear to that?

@jan-auer jan-auer requested a review from mitsuhiko April 17, 2021 08:55
@flub
Copy link
Contributor

flub commented Apr 19, 2021

@samsartor CI is happy, the Danger action is trying to be smart and ask for a changelog entry if necessary (there is no changelog for wasm-split, so not needed I'd say). However it fails because it tries to get permission to post to this PR but doesn't have access to the secret of this repo. We should fix that :(

@samsartor
Copy link
Contributor Author

Alas, Sentry is barfing a little bit on our wasm bundle. I think I have a fix for that here: rustwasm/walrus#209. However, this change appears to be working as intended! I'm ready for it to be merged as soon as y'all are.

@jan-auer jan-auer merged commit 3077796 into getsentry:master Apr 21, 2021
@jan-auer
Copy link
Member

Thanks, overriding status checks and adding a changelog entry on master directly.

jan-auer added a commit that referenced this pull request Apr 21, 2021
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