-
Notifications
You must be signed in to change notification settings - Fork 258
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
Invoke wasm-opt only in release mode? #197
Comments
You also may not have wasm-opt on your development machine's PATH at all; I use a container for building the project's release, which does have it. Because of this, wasm-opt breaks dev builds for me unfortunately. |
@WorldSEnder having to maintain multiple Thoughts? Also, @Follpvosten does the above info resolve your issue at all? |
So, I have overlooked what appears to be the deeper issue. Per-pipeline config can not be altered via Trunk.toml. We essentially need a mechanism to be able to state in the I am tempted to say that the easiest path forward is to just make it such that wasm-opt is NEVER invoked for debug builds; however, as soon as we do that someone will come along with a legit use case for why they NEED it enabled for their debug builds. Though this is not a generic pattern for all other pipeline types, perhaps this is an issue where we just need to provide a new option for the pipeline. Something like There is probably a handful of good ways to do this. Thoughts? |
This is drifting into the direction of Otherwise, I'd be fine with this very specific workaround of having both |
I agree; I think it would be nice to have this as a generic secondary property, like a However, I can see that this might be overkill, so for now, data-wasm-opt-release and data-wasm-opt-debug might be preferable. I would be happy either way, as anything is better than what I have currently ( |
Hahaha, @Follpvosten that's an interesting point about PS |
@WorldSEnder perhaps we could create a mechanism where the This would allow for the optional config system to drive more specialized cases, and CI could easily be updated to just point Trunk to the appropriate Thoughts? |
This boils down to rolling your own templating engine, in the end, which means documenting syntax and extending it? Not an endorsement, e.g. tinytemplate seems also lightweight, doesn't pull in a lot of dependencies (besides serde, which trunk already needs) and would probably work for interpolating attributes, as would microtemplate. Thoughts?
Yeah, that'd work, preferably I could also somehow inherit properties from Maybe define the attributes in |
Nice, tinytemplate could definitely work. Especially given that it will serialize the given context object as json. This would allow us to feed it our TBH, I like this better than having to introduce a bunch of new asset specific attributes. As long as we keep this simple, then the templating logic won't become a maintenance burden. If folks start asking for jinja2/gotmpl like functionality, then we've got a problem. @WorldSEnder as far as config file inheritance, #205 should help to address this. |
At that point, I'd vote for mirroring how |
Thinking about this a bit more, I'm tempted to say that we should totally just ignore wasm-opt for debug builds (can anyone think of an actual use case for wasm-opt on a debug binary?), and just apply wasm-opt automatically to release builds. That way users would only have to define the wasm-opt attr once in their index.html, and wouldn't have to toggle between that value for debug vs release. Thoughts? @udoprog would you be interested in adapting your PR to match this behavior? |
Makes absolute sense to me. The current default behavior in 0.12.1 is what I'd set manually, too (0 for debug, 3 for release) and I don't think I'd ever want to run wasm-opt for debug builds (which, by the way, takes muuuuuuch longer than for release builds because LLVM doesn't optimize many things out in debug mode, so wasm-opt has to go through a lot more code). |
This addresses a few pain points where there was no way to reasonably disable wasm-opt use for debug builds while keeping it enabled for release builds. There seems to be very few (if any) use cases for using wasm-opt on a debug build, as such, wasm-opt is now only run on release builds when enabled. closes #197 closes #175
This addresses a few pain points where there was no way to reasonably disable wasm-opt use for debug builds while keeping it enabled for release builds. There seems to be very few (if any) use cases for using wasm-opt on a debug build, as such, wasm-opt is now only run on release builds when enabled. closes #197 closes #175
This addresses a few pain points where there was no way to reasonably disable wasm-opt use for debug builds while keeping it enabled for release builds. There seems to be very few (if any) use cases for using wasm-opt on a debug build, as such, wasm-opt is now only run on release builds when enabled. closes #197 closes #175
I successfully installed wasm-opt from binaryen and can include
data-wasm-opt="4"
in myindex.html
, which brings the wasm size down by a factor of 60% for a simple hello world project in release mode.But since wasm-opt takes quite a bit of time, I don't want to do that in debug mode. Is there a way to only use that option in
--release
mode? The very ugly way is to have a separateindex-release.html
and possiblyTrunk.release.toml
referencing that file.I see several paths forward, e.g. looking for
Trunk.release.toml
(or variants) and treating that as an override to the generalTrunk.toml
, or preprocessing index.html in some way to allow for dynamic replacements.The text was updated successfully, but these errors were encountered: