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

perf(wasm-builder): invert path remapping logic #2902

Merged
merged 1 commit into from
Jul 17, 2023

Conversation

StackOverflowExcept1on
Copy link
Member

@StackOverflowExcept1on StackOverflowExcept1on commented Jul 4, 2023

The ecosystem team can use the "production" profile to remove the username from the wasm binary.

@StackOverflowExcept1on StackOverflowExcept1on added the A0-pleasereview PR is ready to be reviewed by the team label Jul 4, 2023
Copy link
Member

@breathx breathx left a comment

Choose a reason for hiding this comment

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

Apply fixes for @DennisInSky comments

@breathx breathx added A3-gotissues PR occurred to have issues after the review and removed A0-pleasereview PR is ready to be reviewed by the team labels Jul 12, 2023
@StackOverflowExcept1on
Copy link
Member Author

@breathx #2915 will add a panic handler feature to gstd. It makes sense to enable path remapping only when the panic handler is enabled.

If the user uses dependency (such as dapps-gear-lib) that references gstd, the user should disable the panic handler. Otherwise, such dependency will define the panic handler twice, and we will consider it semantically incorrect (also it leads to compile error).

Does it make sense to check transitive dependencies for the gstd/debug function in this case?

@breathx
Copy link
Member

breathx commented Jul 12, 2023

@breathx #2915 will add a panic handler feature to gstd. It makes sense to enable path remapping only when the panic handler is enabled.

If the user uses dependency (such as dapps-gear-lib) that references gstd, the user should disable the panic handler. Otherwise, such dependency will define the panic handler twice, and we will consider it semantically incorrect (also it leads to compile error).

Does it make sense to check transitive dependencies for the gstd/debug function in this case?

Panic handler may not still contain paths to remap, isn't it?

@StackOverflowExcept1on StackOverflowExcept1on changed the title perf(wasm-builder): parse Cargo.toml before remapping path perf(wasm-builder): do path remapping for production builds only Jul 12, 2023
@StackOverflowExcept1on StackOverflowExcept1on added A0-pleasereview PR is ready to be reviewed by the team and removed A3-gotissues PR occurred to have issues after the review labels Jul 12, 2023
@StackOverflowExcept1on StackOverflowExcept1on changed the title perf(wasm-builder): do path remapping for production builds only perf(wasm-builder): check that path-remapping feature is enabled Jul 13, 2023
@StackOverflowExcept1on StackOverflowExcept1on changed the title perf(wasm-builder): check that path-remapping feature is enabled perf(wasm-builder): invert path remapping logic Jul 15, 2023
@StackOverflowExcept1on StackOverflowExcept1on merged commit e6bcd53 into master Jul 17, 2023
10 checks passed
@StackOverflowExcept1on StackOverflowExcept1on deleted the av/remap-path-prefix-perf branch July 17, 2023 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0-pleasereview PR is ready to be reviewed by the team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants