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

Check meta before adding new custom section #1707

Closed
wants to merge 4 commits into from

Conversation

elizabethengelman
Copy link
Contributor

@elizabethengelman elizabethengelman commented Nov 6, 2024

What

Closes #1694

Why

[TODO: Why this change is being made. Include any context required to understand the why.]

Known limitations

[TODO or N/A]

@Ifropc Ifropc force-pushed the fix/meta-on-build-dupes branch from a27c334 to 61438e5 Compare November 7, 2024 05:22
@Ifropc Ifropc force-pushed the fix/meta-on-build-dupes branch from 61438e5 to 58fa0bc Compare November 7, 2024 05:28
@Ifropc
Copy link
Contributor

Ifropc commented Nov 7, 2024

Update: added a test to reproduce, have been trying to delete a wasm file in release/<contract>.wasm and release/deps/<contract>.wasm for metadata to still persists in between builds 🤔
Will dig deeper into that. Using rustc + cargo 1.82 on 2 different Linux distros (not sure if that matters)

@elizabethengelman elizabethengelman changed the title Wip - check meta before adding new custom section Check meta before adding new custom section Nov 7, 2024
@elizabethengelman
Copy link
Contributor Author

Thank you for adding that test @Ifropc!

@elizabethengelman elizabethengelman marked this pull request as ready for review November 7, 2024 22:30
@elizabethengelman elizabethengelman requested a review from a team as a code owner November 7, 2024 22:30
@leighmcculloch
Copy link
Member

This fix appears to address the symptom, avoid adding new meta when the meta is already, rather than addressing the issue that when building on linux without changes the .wasm file doesn't seem to be rewritten. I think we need to further understand what's going on and why before we patch this at another level.

It's also occurred to me while rereading this code that the cli is writing a whole new section per entry, but it should be joining the new entries together and writing them in one new entry, otherwise the wasm is unnecessarily larger due to managing one section per meta.

As an optimisation I think this change would be worthwhile, but after addressing the second paragraph above because as part of addressing it, it'll need to be solved a different way.

@Ifropc
Copy link
Contributor

Ifropc commented Nov 9, 2024

BTW the current fix doesn't work because it only checks for meta in the wasm file with the same name. (So specifying a different name after recompiling will keep old irrelevant meta). Ideally it shouldn't even be there in a first place, and I'm looking into why it's there, though it seems that it might be coming somewhere from cargo/rustc itself

@elizabethengelman
Copy link
Contributor Author

This fix appears to address the symptom, avoid adding new meta when the meta is already, rather than addressing the issue that when building on linux without changes the .wasm file doesn't seem to be rewritten. I think we need to further understand what's going on and why before we patch this at another level.

Good call - that makes sense. I'll close this PR as we understand the root cause.

It's also occurred to me while rereading this code that the cli is writing a whole new section per entry, but it should be joining the new entries together and writing them in one new entry, otherwise the wasm is unnecessarily larger due to managing one section per meta.

Good point! Does it make sense to create a new issue for this?

@leighmcculloch
Copy link
Member

Opened a new issue:

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.

stellar tx build --meta creates multiple entries
3 participants