Create asbuild tool to fix passing asc parameters on the npm run command line #975

jhwgh1968 · 2019-11-26T02:24:44Z

Causes projects generated by asinit to allow passing arguments to the asc command line when doing an npm run asbuild via an environment variable.

This is to make passing things like --use abort= and --noAssert easier.

I tried using the standard npm argument parsing at first, but it behaved very strangely.

dcodeIO · 2019-11-30T15:54:58Z

Currently, the recommended way to specify such options is to edit the asbuild:* steps in package.json directly, adapting them to your needs. Could you explain why this isn't an option in your scenario?

jhwgh1968 · 2019-11-30T19:10:22Z

It is an option. This PR was just an offer for a simpler path for applying parameters temporarily or experimenting with them. I also admit that I am not very familiar with npm-based packages, and it was an attempt to make the build commands behave similar to those I was familiar with.

I say simpler, because this was not a trivial thing for me to figure out. There is much weirdness in packages.json, and the way its script parameters are executed. Aside from being led astray by examples on StackOverflow, I find it very counter-intuitive that the "obvious" way to pass parameters to a build does not work:

npm run asbuild -- --use abort=

One parameter (such as --noAssert) silently does not actually appear on the asc command line, and more than one is split in a weird way and bash tries to execute some of the pieces (which of course fail).

All that said: if editing package.json is the recommended way, I would be happy to let this be closed, and open a PR against the documentation instead with an improved explanation.

I just want to save others the exercise I went through.

dcodeIO · 2019-11-30T19:17:31Z

From a quick test is appears that if there is

  "scripts": {
    "asbuild:untouched": "asc ...",
    "asbuild:optimized": "asc...",
    "asbuild": "npm run asbuild:untouched && npm run asbuild:optimized"
  }

with

npm run asbuild:optimized -- --measure

one will get

asc ... --measure

but with

npm run asbuild -- --measure

one will get

npm run asbuild:untouched && npm run asbuild:optimized --measure

which is a parameter to npm. Consequently

npm run asbuild -- -- --measure

yields

npm run asbuild:untouched && npm run asbuild:optimized -- --measure

which does what one wants, but only for the second command. Wondering if we can improve the asbuild entry instead to properly propagate arguments.

jhwgh1968 · 2019-11-30T19:25:43Z

Ah, I see. It was the first command I was looking at, and thought it did nothing.

In that case, I will poke around a little more and try to fix that. I did figure out how the scripts were passed to bash (as basically standard input, which means certain things about the argument variables and quoting), but that was the one mystery.

jhwgh1968 · 2019-11-30T20:00:10Z

With that piece of the puzzle, I think I did it. Ready for review!

dcodeIO · 2019-11-30T20:03:40Z

bin/asinit

@@ -210,7 +210,7 @@ function ensurePackageJson() {
  const entryPath = path.relative(projectDir, entryFile).replace(/\\/g, "/");
  const buildUntouched = "asc " + entryPath + " -b build/untouched.wasm -t build/untouched.wat --sourceMap --validate --debug";
  const buildOptimized = "asc " + entryPath + " -b build/optimized.wasm -t build/optimized.wat --sourceMap --validate --optimize";
-  const buildAll = "npm run asbuild:untouched && npm run asbuild:optimized";
+  const buildAll = "function build_all() { npm run asbuild:untouched -- $* && npm run asbuild:optimized -- $*; }; build_all";


This looks like it'll break on Windows :(

Wait, Windows? I thought this required a POSIX shell (i.e. Cygwin or WSL). Even the previous version used && which I didn't think was understood by traditional cmd.exe.

Yeah, there's not much bash and cmd have in common, but && works :)

In that case, would you accept executing a new .js file in node? It seems like overkill, but I don't know of any other way to support Windows...

I guess it could be implement via concurrently package and some extra script like here: https://github.com/kimmobrunfeldt/concurrently/issues/33#issuecomment-433084589

But it's extra dependency and isn't it too overcomplicated? Hmm

jhwgh1968 · 2019-12-01T02:08:24Z

Since I currently have no way to test Windows myself, I though it wise to write a separate PR (to land first) which added a Windows builder to GitHub Actions.

It turned out to be easy to add, but the new builder hit a rather strange error in the nvm version from @dcodeIO 's repo (note this log is from a PR against my own fork):

https://github.com/jhwgh1968/assemblyscript/pull/2/checks?check_run_id=327529556

It seems something strange happened with the nvm alias command, resulting in its standard error being fed into some other parser. Unfortunately, I don't know what it's trying to do, so I can't make sense of the problem with slashes.

Any ideas?

dcodeIO · 2019-12-01T12:51:23Z

nvm has no Windows support, unfortunately. The reason for using a custom action there is that we need aliases for "latest node stable" (node) and "latest node lts" (lts/*), which other solutions that would work on Windows do not yet support. That's a bit of a mess currently. Perhaps we can make our own action that replaces nvm and has aliases?

Most notable differences on Windows are that env variables take the form %MYVAR% instead of $MYVAR, one can't MYVAR=something but must set MYVAR=something and that we can't rely on bash features other than && and || and whatever else just so happens to work on both.

MaxGraey · 2019-12-01T12:54:21Z

Most notable differences on Windows are that env variables take the form %MYVAR% instead of $MYVAR

I guess this could be normalize via cross-env

dcodeIO · 2019-12-01T12:57:23Z

I guess this could be normalize via cross-env

Seen this, yeah. But unfortunate that we'd need a dependency just for that.

jhwgh1968 · 2019-12-01T15:36:52Z

I also found a version of nvm that claims to support windows:
https://www.npmjs.com/package/nvm-win

It is pretty old, and there is a patch that seems like a good idea to apply:
joney-pinkman/nvm-win#2

But it might work. Is that a good way forward, @dcodeIO?

EDIT: This one is recommended by the README and seems newer:
https://github.com/coreybutler/nvm-windows

MaxGraey · 2019-12-01T15:40:37Z

This more maintainable https://github.com/coreybutler/nvm-windows but it require Go 😂

jhwgh1968 · 2019-12-01T15:45:29Z

It requires Go to build from source, but the project has binary releases. Since this is only being used on a Windows builder, I was thinking about just downloading and unzipping it in the GitHub Action, without any changes to project dependencies.

MaxGraey · 2019-12-01T15:49:40Z

yes, hope prebuild release works on windows in ci

jhwgh1968 · 2019-12-01T16:49:27Z

Success!

Windows CI PR: #984

jhwgh1968 · 2019-12-06T01:15:11Z

With the Windows CI PR in place, I wrote a first draft of the approach I suggested earlier. Thinking about @MaxGraey's concerns, I did not add any dependencies.

Javascript is not a language I am very good with, so I expect it will require clean up and more testing. I am most interested in feedback about whether this approach is acceptable.

jhwgh1968 · 2019-12-09T02:28:12Z

Ping @dcodeIO @MaxGraey, what do you think of this new version so far?

dcodeIO · 2019-12-09T06:42:34Z

Being somewhat hesitant on adding a file just for this to the scaffold. Perhaps a cleaner strategy could be to provide an asbuild tool similar to asc and asinit that is aware of the build steps being named asbuild, asbuild:untouched and asbuild:optimized that then distributes parameters? Like, calling

asbuild [additionalOptions...]

would replace

npm run asbuild -- [additionalNotWorkingOptions...]

Ultimately, one can choose either of these ofc, with asbuild supporting even more stuff we add over time?

Additional stuff we might want to add in follow-ups might be

Compile both untouched and optimized in parallel
A --save flag that saves the specified options
Adding entire alternative configurations (of the form asbuild:CONFIGNAME like the default untouched and optimized
A --list flag that lists the options of one or all steps

Another nice side-effect of this is that we don't have to come up with our own configuration options format stuffed into tsconfig.json since asbuild can just work off npm scripts.

jhwgh1968 · 2019-12-15T01:53:18Z

I've been working on this, but it's not quite ready yet. See if my new approach is better:

Create a new executable called asbuild, which is installed along with asc and asinit.
It takes optional parameters --optimized and --untouched. If neither is specified, it creates "untouched" only. If both are specified, it builds both -- currently in series, perhaps later in parallel.
If there is a -- on its command line, options after that are passed to asc. (This is to allow adding more options to asbuild itself in the future.)
The package.json in projects still has the same targets they do now.
The project targets just call asbuild with different options based on the target. For example, the plain "asbuild" target -- which builds both, and currently does not pass parameters correctly -- would have this line: bin/asbuild --optimized --untouched

Is that more what you were thinking, @dcodeIO?

MaxGraey · 2019-12-15T02:06:39Z

p. 2. make all things much complicated. Like if we want optimize build we should use "--optimize" and "--optimized" at the same time which looks verbose. In other hand if we want untouched with --debug and optimized without --debug in the same time it will be look even worst like bunch of flags: "--untouched", "--debug", "--optimized", "--no-debug", "--optimize". And "--debug" could use also for optimize build but in this case all of this not obvious

jhwgh1968 · 2019-12-15T04:47:35Z

Yes, that is a good point.

I was originally trying to make the smallest change possible, rather than thinking bigger picture. In that case, how about I take a page from the Rust language's book, and organize the options into "profiles"?

That is, rather than setting all the settings asbuild needs -- should debug be generated, should optimizations be run, where does the output go -- the defaults should be set in pre-defined groups called "profiles", based on the intention of the user.

This is more or less what the current scripts do with "untouched" versus "optimized". This tool would just make that more formal, and allow it to expand in a more natural way over time.

Here is the output from my current WIP command (which uses the yargs parser) to illustrate what I mean:

$ bin/asbuild 
Build Profiles:
  --debug    build a debug wasm file: +debuginfo -optimizations
  --release  build a release wasm file: -debuginfo +optimizations

Options:
  --help         Show help                                             [boolean]
  --version      Show version number                                   [boolean]
  --pkgroot      the built package's root; contains package.json      [required]
  --output-wasm  override the destination wasm file for the first build
  --output-wat   override the destination wat file for the first build

Missing required argument: pkgroot

I copied the profile names from Rust's build system, cargo. (Since that is largely modeled on npm, I thought that was fair. 😄 )

Notice they are words describing the end product -- "a build for release" -- rather than words describing its contents -- "a build that is optimized", "a build with debug symbols". I think this addresses your point, @MaxGraey.

The only required argument is --pkgroot (just because I couldn't figure out how npm would invoke it). Otherwise, the profiles decide everything unless the user overrides things. It is possible to specify more than one profile with --debug --release but custom output names will only affect the first one (to avoid conflicts).

Only commands in package.json should invoke this tool most of the time, but should still be usable by a person to do something custom in their own project build system. (Again, this is because I am keeping my own future uses in mind.)

Right now, I am following current file names: "untouched" is the default name for the debug profile, and "optimized" is the default name for the release profile. But I can easily change that if you want.

How does that sound?

jhwgh1968 · 2019-12-20T01:01:54Z

The CI failure is a lock file update. Any comments on the approach, @dcodeIO or @MaxGraey?

MaxGraey · 2019-12-20T01:03:27Z

you should sync with master and make npm install

jhwgh1968 · 2019-12-21T00:02:40Z

Thanks, @MaxGraey.

jhwgh1968 · 2020-01-02T01:04:56Z

Now that my CI issue is fixed, is this more like what you were expecting, @dcodeIO? I will probably finish it up this week if so.

Also, I did have to add one dependency to avoid a lot of frustration.

dcodeIO · 2020-01-02T02:27:58Z

I like the idea of having release and debug profiles in there, maybe that's something we should pursue over optimized/untouched in general. I guess 99% of users are interested in either of the following:

A debug build that compiles quickly (debug, currently --debug)
A release build optimized for speed (release, currently -O3 --noAssert)
A release build optimized for size (releasemin, currently -O3z --noAssert?)

So perhaps that'd be good profiles to begin with? These could live in cli/asbuild/profiles as JSON files or something.

Now I'm always cautious when dependencies are added, mostly because it's hard to keep track of dependencies of dependencies which can become a security nightmare or package bloat. Like, we have just binaryen and long in release dependencies due to that, two packages without any dependencies that are controlled by me. In the case of options parsing we already have cli/util/options.js btw. It's not as sophisticated as yargs or other full-blown command line option parsers, but it served us well so far as the options parser for asc with its options in cli/asc.json.

Apart from that I suggest to keep option names somewhat close to those of asc, here --pkgroot and --output-wasm, which are --baseDir respectively --binaryFile/--textFile/--asmjsFile/--idlFile/--tsdFile with the respective aliases in asc. Doesn't match super well, so that might need some rethinking as well?

jhwgh1968 · 2020-01-03T22:47:19Z

Thanks for the feedback, @dcodeIO. Based on that, I'll make some fixes, and the next commit should be ready for merge.

jhwgh1968 · 2020-02-11T04:39:11Z

Alright, @dcodeIO, I think I've got it pretty much done. I think I have taken all your design suggestions, and avoided adding any dependencies.

Ready for review!

jhwgh1968 · 2020-02-22T01:32:03Z

Ping @dcodeIO or @MaxGraey, this is ready for review!

jhwgh1968 changed the title ~~Add ASC_FLAGS env variable~~ [WIP] Fix command passing asc parameters on the npm run command line Nov 30, 2019

jhwgh1968 changed the title ~~[WIP] Fix command passing asc parameters on the npm run command line~~ [WIP] Fix passing asc parameters on the npm run command line Nov 30, 2019

jhwgh1968 changed the title ~~[WIP] Fix passing asc parameters on the npm run command line~~ Fix passing asc parameters on the npm run command line Nov 30, 2019

dcodeIO reviewed Nov 30, 2019

View reviewed changes

jhwgh1968 changed the title ~~Fix passing asc parameters on the npm run command line~~ Create asbuild tool to fix passing asc parameters on the npm run command line Jan 2, 2020

Created new asbuild utility, run from npm

5d391ba

jhwgh1968 closed this Feb 29, 2020

jhwgh1968 mentioned this pull request Feb 29, 2020

Add asbuild utility, update asinit to use it #1134

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create asbuild tool to fix passing asc parameters on the npm run command line #975

Create asbuild tool to fix passing asc parameters on the npm run command line #975

jhwgh1968 commented Nov 26, 2019

dcodeIO commented Nov 30, 2019

jhwgh1968 commented Nov 30, 2019

dcodeIO commented Nov 30, 2019

jhwgh1968 commented Nov 30, 2019

jhwgh1968 commented Nov 30, 2019

dcodeIO Nov 30, 2019

jhwgh1968 Dec 1, 2019

dcodeIO Dec 1, 2019

jhwgh1968 Dec 1, 2019

MaxGraey Dec 1, 2019

jhwgh1968 commented Dec 1, 2019

dcodeIO commented Dec 1, 2019 •

edited

Loading

MaxGraey commented Dec 1, 2019

dcodeIO commented Dec 1, 2019

jhwgh1968 commented Dec 1, 2019 •

edited

Loading

MaxGraey commented Dec 1, 2019

jhwgh1968 commented Dec 1, 2019

MaxGraey commented Dec 1, 2019

jhwgh1968 commented Dec 1, 2019 •

edited

Loading

jhwgh1968 commented Dec 6, 2019 •

edited

Loading

jhwgh1968 commented Dec 9, 2019

dcodeIO commented Dec 9, 2019 •

edited

Loading

jhwgh1968 commented Dec 15, 2019

MaxGraey commented Dec 15, 2019 •

edited

Loading

jhwgh1968 commented Dec 15, 2019

jhwgh1968 commented Dec 20, 2019

MaxGraey commented Dec 20, 2019

jhwgh1968 commented Dec 21, 2019

jhwgh1968 commented Jan 2, 2020

dcodeIO commented Jan 2, 2020 •

edited

Loading

jhwgh1968 commented Jan 3, 2020

jhwgh1968 commented Feb 11, 2020

jhwgh1968 commented Feb 22, 2020 •

edited

Loading

Create asbuild tool to fix passing asc parameters on the npm run command line #975

Create asbuild tool to fix passing asc parameters on the npm run command line #975

Conversation

jhwgh1968 commented Nov 26, 2019

dcodeIO commented Nov 30, 2019

jhwgh1968 commented Nov 30, 2019

dcodeIO commented Nov 30, 2019

jhwgh1968 commented Nov 30, 2019

jhwgh1968 commented Nov 30, 2019

dcodeIO Nov 30, 2019

Choose a reason for hiding this comment

jhwgh1968 Dec 1, 2019

Choose a reason for hiding this comment

dcodeIO Dec 1, 2019

Choose a reason for hiding this comment

jhwgh1968 Dec 1, 2019

Choose a reason for hiding this comment

MaxGraey Dec 1, 2019

Choose a reason for hiding this comment

jhwgh1968 commented Dec 1, 2019

dcodeIO commented Dec 1, 2019 • edited Loading

MaxGraey commented Dec 1, 2019

dcodeIO commented Dec 1, 2019

jhwgh1968 commented Dec 1, 2019 • edited Loading

MaxGraey commented Dec 1, 2019

jhwgh1968 commented Dec 1, 2019

MaxGraey commented Dec 1, 2019

jhwgh1968 commented Dec 1, 2019 • edited Loading

jhwgh1968 commented Dec 6, 2019 • edited Loading

jhwgh1968 commented Dec 9, 2019

dcodeIO commented Dec 9, 2019 • edited Loading

jhwgh1968 commented Dec 15, 2019

MaxGraey commented Dec 15, 2019 • edited Loading

jhwgh1968 commented Dec 15, 2019

jhwgh1968 commented Dec 20, 2019

MaxGraey commented Dec 20, 2019

jhwgh1968 commented Dec 21, 2019

jhwgh1968 commented Jan 2, 2020

dcodeIO commented Jan 2, 2020 • edited Loading

jhwgh1968 commented Jan 3, 2020

jhwgh1968 commented Feb 11, 2020

jhwgh1968 commented Feb 22, 2020 • edited Loading

dcodeIO commented Dec 1, 2019 •

edited

Loading

jhwgh1968 commented Dec 1, 2019 •

edited

Loading

jhwgh1968 commented Dec 1, 2019 •

edited

Loading

jhwgh1968 commented Dec 6, 2019 •

edited

Loading

dcodeIO commented Dec 9, 2019 •

edited

Loading

MaxGraey commented Dec 15, 2019 •

edited

Loading

dcodeIO commented Jan 2, 2020 •

edited

Loading

jhwgh1968 commented Feb 22, 2020 •

edited

Loading