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

--skip-build flag #144

Closed
ashleygwilliams opened this issue Jun 5, 2018 · 10 comments
Closed

--skip-build flag #144

ashleygwilliams opened this issue Jun 5, 2018 · 10 comments
Labels
good first issue Good for newcomers PR attached there's a PR open for this issue to-do stuff that needs to happen, so plz do it k thx
Milestone

Comments

@ashleygwilliams
Copy link
Member

while teaching wasm-pack, i discovered that sometimes we want to run wasm-pack init again (updated readme or cargo.toml) but don't want to rebuild the whole pkg

let's add a --skip-build flag that will skip:

  • adding target
  • compiling to wasm
  • installing wasm-bindgen
  • running wasm-bindgen

it should warn if the wasm file does not exist in target (was not yet compiled) or if the bg.wasm or js module wrapper are not present (hasnt yet run wasm-bingden)

@ashleygwilliams ashleygwilliams added help wanted Extra attention is needed good first issue Good for newcomers to-do stuff that needs to happen, so plz do it k thx labels Jun 5, 2018
@ashleygwilliams ashleygwilliams added this to the 0.4.0 milestone Jun 5, 2018
@mgattozzi
Copy link
Contributor

One important thing to note is that the order of what happens has a specific number of steps hardcoded in the output. We'll need to add checks to choose the right amount if this flag is added. i.e. instead of say step [3/7] with this flag we might just need to do [2/3]

@ashleygwilliams
Copy link
Member Author

@mgattozzi good call. i mean, this is probably a good reason to stop hardcoding that stuff in the first place. the more steps and flags we add the more likely we'll want to have a dynamic way to set up a list of steps we want and then count them from there.

@mgattozzi
Copy link
Contributor

@ashleygwilliams I'm thinking maybe an enum of some sort that can be matched against to choose the right number. This kind of brings us to the context type in #123. I think the app and the granularity we need is too big to pass in individual arguments anymore. But yeah this should be more dynamic not hardcoded

@ashleygwilliams ashleygwilliams added PR attached there's a PR open for this issue and removed help wanted Extra attention is needed labels Jun 5, 2018
@itsybitesyspider
Copy link
Contributor

itsybitesyspider commented Jun 7, 2018

First apologies. Just noticed this issue. I'm aware there's already a pull request. This flag simultaneously does something I really want and also has qualities that make it less useful.

Parts of this flag that I really like: "Adding the target" and "installing wasm-bindgen" affect the user's global environment. That always has a lot of potential to cause surprise or breakage. Especially if I give anyone (coworker or stranger on github) a build script that uses wasm-pack as an intermediate step I have to be very careful to educate them about what is going to happen (and some people would interpret an indirectly-commanded software install as malware, eep!). Also these steps are slow. So suppressing these is a big win except when they're wanted.

Parts of this flag that feel regrettable: Having disabled the above, I still want "Compiling to wasm" and "running wasm-bindgen". And these are relatively fast operations. In --debug mode, we get incremental compilation, and even in release mode there seems to at least be some kind of short-circuit behavior. So, AFAICS, if someone is saving time by skipping these steps, then that itself is evidence that their output is stale and probably wrong and not what they intend.

I put some times below; emphasis on the very last result, which is sub-second despite apparently rebuilding a 1.4M wasm file. I actually re-run wasm-pack in some cases 10-20 times in an hour if I'm fine-tuning something, so I'm intensely interested in these times.

I hope this is helpful.

(Note to avoid confusing anyone who comes across this ticket: as of this writing, the --debug flag lives in a pull request and is not yet a part of wasm-pack.)

developer server-wasm> time ../../../wasm-pack/target/debug/wasm-pack init --debug
| [1/7] Adding WASM target...
| [2/7] Compiling to WASM...
| [3/7] Creating a pkg directory...
| [4/7] Writing a package.json...
| [5/7] Copying over your README...
| :-) [WARN]: origin crate has no README
| [6/7] Installing WASM-bindgen...
| [7/7] Running WASM-bindgen...
| :-) Done in 2 minutes
| :-) Your WASM pkg is ready to publish at ./pkg.

real    2m27.291s
user    13m29.196s
sys     0m14.444s

developer server-wasm> time ../../../wasm-pack/target/debug/wasm-pack init --debug
| [1/7] Adding WASM target...
| [2/7] Compiling to WASM...
| [3/7] Creating a pkg directory...
| [4/7] Writing a package.json...
| [5/7] Copying over your README...
| :-) [WARN]: origin crate has no README
| [6/7] Installing WASM-bindgen...
| [7/7] Running WASM-bindgen...
| :-) Done in 1 minute
| :-) Your WASM pkg is ready to publish at ./pkg.

real    1m45.961s
user    11m2.164s
sys     0m9.020s
developer server-wasm> make clean
cargo clean
rm -rf pkg

developer server-wasm> time ../../../wasm-pack/target/debug/wasm-pack init        
| [1/7] Adding WASM target...
| [2/7] Compiling to WASM...
| [3/7] Creating a pkg directory...
| [4/7] Writing a package.json...
| [5/7] Copying over your README...
| :-) [WARN]: origin crate has no README
| [6/7] Installing WASM-bindgen...
| [7/7] Running WASM-bindgen...
| :-) Done in 3 minutes
| :-) Your WASM pkg is ready to publish at ./pkg.

real    3m3.534s
user    17m53.744s
sys     0m14.868s

developer server-wasm> time ../../../wasm-pack/target/debug/wasm-pack init
| [1/7] Adding WASM target...
| [2/7] Compiling to WASM...
| [3/7] Creating a pkg directory...
| [4/7] Writing a package.json...
| [5/7] Copying over your README...
| :-) [WARN]: origin crate has no README
| [6/7] Installing WASM-bindgen...
| [7/7] Running WASM-bindgen...
| :-) Done in 1 minute
| :-) Your WASM pkg is ready to publish at ./pkg.

real    1m44.169s
user    11m0.016s
sys     0m8.960s

At this point, I rebuild wasm-pack to never run rustup or cargo install wasm-bindgen:

developer server-wasm> make clean
cargo clean
rm -rf pkg

developer server-wasm> time ../../../wasm-pack/target/debug/wasm-pack init --debug
| [2/7] Compiling to WASM...
| [3/7] Creating a pkg directory...
| [4/7] Writing a package.json...
| [5/7] Copying over your README...
| :-) [WARN]: origin crate has no README
| [7/7] Running WASM-bindgen...
| :-) Done in 37 seconds
| :-) Your WASM pkg is ready to publish at ./pkg.

real    0m37.904s
user    2m26.384s
sys     0m5.240s

developer server-wasm> time ../../../wasm-pack/target/debug/wasm-pack init --debug
| [2/7] Compiling to WASM...
| [3/7] Creating a pkg directory...
| [4/7] Writing a package.json...
| [5/7] Copying over your README...
| :-) [WARN]: origin crate has no README
| [7/7] Running WASM-bindgen...
| :-) Done in 0 seconds
| :-) Your WASM pkg is ready to publish at ./pkg.

real    0m0.895s
user    0m0.816s
sys     0m0.080s

developer server-wasm> 

@ashleygwilliams
Copy link
Member Author

ashleygwilliams commented Jun 12, 2018

hey @clanehin i think that this is incredibly useful and different "build profiles", or sets of wasm-pack steps, is something that i am also doing a lot of thinking about right now. the fact that wasm-pack currently affects the global env is something i'm not pleased about and is something we are actively discussing mitigating in #146 (at least for the wasm-bindgen dep, i'm not sure how we could mitigate it for the target but i'm open to suggestions!)

so one question for you- do you have a suggestion about improving the current PR? or do you think perhaps what you are suggestion is another build type flag? e.g. a --no-installs type flag? i definitely know the use case here and think it would be very useful.

mostly- i think what you're saying here is very valuable but i'm not sure, and am curious, exactly what your ask is!

@itsybitesyspider
Copy link
Contributor

exactly what your ask is

I guess I left it out in the spirit of, "let me show you my data and see if you come to same conclusion I did."

So, I hope I've convinced you that we don't need to skip building the output (wasm and package.json and support javascript). I also see two disadvantages: as a pedagogical tool, it risks creating extra novelty for the learner to keep track of, and as an everyday-use tool it risks leaving the user with stale output.

So we could rename --skip-build to --no-installs, and just not disable the build (but do disable touching the environment) and call it done. That isn't very much work over the existing PR and gives us what we need.

I've also thought about a couple of other ways to bikeshed this, like having a separate command wasm-pack init-environment that the user runs only when they care about that sort of thing.

Alternately, we could have a readline interface "Would you like to install the wasm-unknown-unknown target? [Y/n]" And then provide the flags --yes, --no to suppress asking the questions. Users who run wasm-pack without knowing what it does will be alerted to what it does before it does it, and I noticed that a friend who was learning Linux for the first time tolerated the Debian Installer's incessant questioning rather well, so maybe it won't be so bad for new users?

@ashleygwilliams
Copy link
Member Author

@clanehin so- i think all of this is really great and i think i would like to open up a new issue to get a --no-install flag implemented. my one thought is: do you think that --skip-build existing is actively harmful or detrimental to the features you want? since we are 1.0 i think it's potentially useful to play with some of these "build coonfiguration options" and see how they play out in the wild. to me, having both a --skip-build and --no-install is not a problem... though i'm curious what you think?

mostly, i think i would like to merge the --skip-build PR - it has some excellent refactoring in it that make this build configuration work very easy and will open us up to explore the space more. if it turns out we end up wanting to deprecate --skip-build, i dont think that is a huge issue. curious your thoughts?

@ashleygwilliams
Copy link
Member Author

ok- i am going to make a sort of executive call to merge the current PR- but please don't view this as me dismissing or diminishing your comments. i think if we need to we can still deprecate and/or change --skip-build. i'd like to get this refactor in now to unblock a bunch of PRs. i'll start up a new issue to discuss this stuff :)

ashleygwilliams added a commit that referenced this issue Jun 13, 2018
@ashleygwilliams
Copy link
Member Author

closed by #151

@itsybitesyspider
Copy link
Contributor

it has some excellent refactoring

I noticed. Entirely sensible; if anyone still feels they would benefit from --skip-build I don't see any reason to deprive them of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers PR attached there's a PR open for this issue to-do stuff that needs to happen, so plz do it k thx
Projects
None yet
Development

No branches or pull requests

3 participants