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

Support asconfig.json in addition to CLI arguments #1230

Closed
jtenner opened this issue Apr 17, 2020 · 19 comments
Closed

Support asconfig.json in addition to CLI arguments #1230

jtenner opened this issue Apr 17, 2020 · 19 comments

Comments

@jtenner
Copy link
Contributor

jtenner commented Apr 17, 2020

In an attempt to standardize the way the vscode language server works, we should probably adopt a configuration file definition.

Please file suggestions and discussion on this topic here.

I plan on starting work on this tomorrow in a pull request.

@dcodeIO
Copy link
Member

dcodeIO commented Apr 17, 2020

This reminds me that #1134 is somewhat related in that it proposes an asbuild cli utility, with profiles for debug, release or custom. Perhaps there are ways to make these two work well together?

Another nice touch might be to allow the contents of asconfig.json to be, as a fallback if it's not present, part of tsconfig.json as the assembly key. How useful that'll be ultimately depends a bit, but since we have portable AssemblyScript that compiles to both JS and Wasm, which most likely won't go away, it seems good to have.

@jtenner
Copy link
Contributor Author

jtenner commented Apr 18, 2020

EDIT: I will try to keep this updated with the latest iteration as per suggestions.

type EnableFeatures = "sign-extension" | "bulk-memory" | "simd" | "threads" | "reference-types";
type DisableFeatures = "mutable-globals";
type TrapMode = "js" | "clamp" | "allow";
type BinaryenPasses = "AlignmentLowering" |
  "asyncify" |
  "avoid-reinterprets" |
  "coalesce-locals" |
  "coalesce-locals-with-learning" |
  "code-folding" |
  "code-pushing" |
  "const-hoisting" |
  "dae" |
  "dae-optimizing" |
  "data-flow-opts" |
  "dead-code-elimination" |
  "directize" |
  "dward-dump" |
  "duplicate-import-elimination" |
  "duplicate-function-elimination" |
  "emit-target-features" |
  "extract-function" |
  "flatten" |
  "func-cast-emulation" |
  "full-printer" |
  "function-metrics" |
  "generate-stack-ir" |
  "i64-to-i32-Lowering" |
  "inline-main" |
  "inlining" |
  "inlining-optimizing" |
  "legalize-js-interface" |
  "legalize-js-interface-minimally" |
  "limit-segments" |
  "local-cse" |
  "log-execution" |
  "instrument-locals" |
  "instrument-memory" |
  "loop-invariant-code-motion" |
  "memory-packing" |
  "merge-blocks" |
  "merge-locals" |
  "minified-printer" |
  "minify-imports" |
  "minify-imports-and-exports" |
  "minify-imports-and-exports-and-modules" |
  "metrics" |
  "name-list" |
  "no-rxit-runtime" |
  "optimize-added-constants" |
  "optimize-added-constants-propagate" |
  "optimize-instructions" |
  "optimize-stack-ir" |
  "pick-load-signs" |
  "mod-asyncify-always-only-unwind" |
  "mod-asyncify-never-unwind" |
  "post-assemblyscript" |
  "post-assemblyscript-finalize" |
  "post-emscripten" |
  "precompute" |
  "precompute-propagate" |
  "printer" |
  "print-call-graph" |
  "print-features" |
  "print-function-map" |
  "print-stack-ir" |
  "relooper-jump-threading" |
  "remove-non-js-ops" |
  "remove-imports" |
  "remove-memory" |
  "remove-unused-brs" |
  "remove-unused-module-elements" |
  "remove-unused-non-function-module-elements" |
  "remove-unused-names" |
  "reorder-functions" |
  "reorder-locals" |
  "re-reloop" |
  "redundant-set-elimination" |
  "round-trip" |
  "safeHeap" |
  "simplify-locals" |
  "simplify-globals" |
  "simplify-globals-optimizing" |
  "simplify-locals-no-nesting" |
  "simplify-locals-no-tee" |
  "simplify-locals-no-structure" |
  "simplify-locals-no-tee-no-structure" |
  "strip-debug" |
  "strip-dwarf" |
  "strip-producers" |
  "strip-target-features" |
  "souperify" |
  "souperify-single-use" |
  "spill-pointers" |
  "ssaify" |
  "ssaify-no-merge" |
  "trap-mode-c" |
  "trap-mo" |
  "untee" |
  "vacuum";

interface ASConfig {
  options?: {
    optimizeLevel?: 0 | 1 | 2 | 3;
    shrinkLevel?: 0 | 1 | 2 | "s" | "z";
    converge?: boolean;
    noAssert?: boolean;
    runtime?: "full" | "half" | "stub" | "none";
    debug?: boolean;
    importMemory?: boolean;
    sharedMemory?: boolean;
    importTable?: boolean;
    exportTable?: boolean;
    explicitStart?: boolean; 
    enable?: EnableFeatures[];
    disable?: DisableFeatures[];
    use?: {
      [key: string]: string | number;
    };
    memoryBase?: number;
    tableBase?: number;
    validate?: boolean;
    trapMode?: TrapMode;
    runPasses?: BinaryenPasses[];
    baseDir?: string;
    noUnsafe?: boolean;
    noEmit?: boolean;
    measure?: boolean;
    pedantic?: boolean;
    lib?: string[];
    path?: string[];
    traceResolution?: boolean;
    printrtti: boolean;
  };
  output?: {
    outFile?: string;
    binaryFile: string;
    textFile?: string;
    asmjsFile?: string;
    idlFile?: string;
    tsdFile?: string;
    sourceMap?: boolean | string;
  };
  entry?: string[];
}

We can use this as a starting point. I went through the CLI help and generated an asconfig.json shape.

Any suggestions?

@dcodeIO
Copy link
Member

dcodeIO commented Apr 18, 2020

Nice! A few comments:

  • sourceMap?: boolean is a string. Relative source map if specified without an argument (--sourceMap, string empty), otherwise absolute source map using the specified URL (--sourceMap URL). Perhaps this can be improved to also take a true instead of an empty string.
  • Binaryen passes are lowercase and use dashes, or do we postprocess these from uppercase camelcase?
  • What do these do? include?: string[]; exclude?: string[]; extends?: string[];

@jtenner
Copy link
Contributor Author

jtenner commented Apr 18, 2020

sourceMap?: boolean is a string. Relative source map if specified without an argument (--sourceMap, string empty), otherwise absolute source map using the specified URL (--sourceMap URL). Perhaps this can be improved to also take a true instead of an empty string.

See edit, switched to boolean | string.

Binaryen passes are lowerdashcase

Got it. Will change it.

What do these do? include?: string[]; exclude?: string[]; extends?: string[];

All of these were copied from the idea of what the tsconfig does, but we have entry points, so instead I replaced them with an entry?: string[] property.

Perhaps there is no need for asconfig extension.

@dcodeIO
Copy link
Member

dcodeIO commented Apr 18, 2020

Have been brainstorming with @jtenner on Slack, and this was the latest we came up with:

Let every project have an asconfig.json in its main directory. Like TypeScript, it has a compilerOptions entry specifying the options used for compilation:

{
  "compilerOptions": {
    ...
  }
}

The configuration can extend another configuration using the extends key, using node resolution:

{
  "extends": "./path/to/other/asconfig.json"
}

Now, while one doesn't typically have the need for multiple builds in TypeScript, it is fairly common to build two variants in AssemblyScript. To make this easier and reduce the amount of files one has to deal with, extending this with multiple targets seems convenient:

{
  "compilerOptions": {
    // common options used by all targets
  },
  "targets": {
    "release": {
      // additional release options
    },
    "debug": {
      // additional debug options
    }
  }
}

This feature would be optional, in that if the targets key is not present, just compilerOptions will be used and function just as one would expect from TypeScript (can have multiple files, one per target if one so wishes). Iff the targets key is present, the default target to build would be the first specified (here: release), issuing a warning on the console "No target specified: Defaulting to 'release'".

On the command line, one would either specify just the config file via --config ./path/to/asbuild.json or, if targets is present, --config ./path/to/asbuild.json:release to execute a single one, --config ./path/to/asbuild.json:release,debug to execute multiple or --config ./path/to/asbuild.json:* to execute all. Similarly, if such a config is extended, one would either

{
  "extends": "./path/to/asconfig.json"
}

or, if targets is present,

{
  "extends": "./path/to/asconfig.json:release"
}

If targets is present and the former is used, so a default target is executed, a warning on the console should read "No target specified for './path/to/asconfig.json': Defaulting to 'release'.".

Thoughts? The obvious alternative would be to omit the targets mechanism, but then tooling might have problems determining which asconfig.x.json is the correct one, unless the default one should always be named asconfig.json with additional ones being possible?

@jtenner
Copy link
Contributor Author

jtenner commented Apr 19, 2020

Specifying the first "key" as the target is undefined behavior in js. Object keys are not garunteed to be ordered.

Suggesting to use an Array instead, and always use the first one.

@willemneal
Copy link
Contributor

Typescript currently allows the extend to use node package resolution.

@willemneal
Copy link
Contributor

Also it would be nice to have nested projects so that multiple binaries can be generated. This is particularly of interest to Near with projects with multiple smart contracts.

@jtenner
Copy link
Contributor Author

jtenner commented Apr 20, 2020

@dcodeIO I am suggesting this:

{
  "options": {},
  "targets": [
    {
      "name": "release",
      "options": { ... }
    }
  ]
}

You cannot default to the "first" one, if it's only object keys. Can we resolve this first before I start work?

@dcodeIO
Copy link
Member

dcodeIO commented Apr 20, 2020

Hmm, with that, we could even do something fancy like:

{
  "targets": [
    {
      "name": "release",
      "extends": "assemblyscript/presets:release",
      "options": { ... }
    },
    {
      "name": "debug",
      "extends": "assemblyscript/presets:debug",
      "options": { ... }
    }
  ]
}

but raises the question whether we still need top-level options?

@jtenner
Copy link
Contributor Author

jtenner commented Apr 21, 2020

Again. Why is it so inconvenient or wrong to just have single asconfig files per target?

asconfig.release.json #extends asconfig.json
asconfig.debug.json #extends asconfig.json
asconfig.json # default options

@dcodeIO
Copy link
Member

dcodeIO commented Apr 21, 2020

My thinking there is that one typically has two builds per project, like one debug and one release. Without targets, it is strictly necessary to have multiple files, like

asconfig.base.json
asconfig.debug.json (extends base)
asconfig.json (extends base, implicit release, default)

where it seems just too easy to overlook something due to having to switch through the files, instead of having a clear view on all the options at once.

Then there is

Also it would be nice to have nested projects so that multiple binaries can be generated

which is impossible to do (in a single command) without targets, since every config makes exactly one binary.

@jtenner
Copy link
Contributor Author

jtenner commented Apr 21, 2020

Right. I'm arguing for simplicity and leaving the complexity for end users.

I am now proposing a recursive configuration algorithm and exactly two cli options with defaults specified below.

--config (path) 
--target (name) 

# or

--config (path)(:name?) 

Steps:

  1. Load config
  2. If the config extends another config, load it and resolve it first using this algoirthm
  3. Override default and parent options at this level, but do not override cli provided options
  4. Override options with the first target entry or by the given name (if provided, not overriding the cli passed options)
  5. Return the cliOptions object

@jtenner
Copy link
Contributor Author

jtenner commented Apr 21, 2020

I'm sorry this is taking so long. Encapsulating this algorithm feels quite complicated. 😅

@jhwgh1968
Copy link
Contributor

jhwgh1968 commented Apr 26, 2020

@jtenner, I am the author of the asbuild PR, who was just sent over here.

I think this newer approach looks promising, but may become easier if we can join our efforts. Allow me to add a little bit of context, and see whether you think my proposal makes sense.

After discovering some rather silly limitations of the current package.json file, and having some discussions with @dcodeIO, I ended up writing a new tool called asbuild. My original basis was the Rust ecosystem, which has a tool called cargo (which was largely based on npm).

Instead of managing packages, this tool is the main interface between the dev and the compiler. When you run asbuild, you tell it two things:

  1. A build profile: general properties of the output artifacts, grouped around what the dev is trying to achieve (debug symbols, optimizations, assertions, etc).
  2. A build target: what artifact is expected to be output: which binary if there is more than one, or assembly versus an executable. (I am new to JS and WebAssembly, so I am not well versed in this area.)

While asbuild would support a set of defaults, per-project settings make sense in an asconfig.json file. It is equivalent in the Rust ecosystem to Cargo.toml, or package.json with npm.

The key is: those settings are their own abstraction. How they work -- what options they pass to the compiler -- should be allowed to change between toolchain versions. The compiler can change its flags, but so long as asbuild changes to match, developers will get what they expect.

Manually adding compiler flags directly may be necessary, so that should be possible, but awkward. Intentionally so. Most of the time, the dev should not be thinking at that level.

To conclude, I think having a single asconfig.json (or perhaps one per target) would serve as great input to asbuild. It would contain all the information necessary for the project -- all the targets supported and non-default options for all profiles -- and let asbuild work out the asc command line to achieve it.

I don't think this affects much of what you have described so far, except perhaps to put more knowledge about the compiler into asbuild, rather than the configuration itself.

My prototype was an MVP, and there is a lot more it could/should do. You seem to have a better grasp of things, so any suggestions or info you have to improve it would help.

@stale
Copy link

stale bot commented May 26, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label May 26, 2020
@jtenner
Copy link
Contributor Author

jtenner commented May 27, 2020

I am taking a break from open source development for a while, and I don't want this issue to die, because it's important to AssemblyScript.

I have a few more thoughts before I leave this issue up for grabs:

  • An MVP asconfig is better than trying to make a fully functional asconfig pull request with asconfig extension
  • configuration shape should be defined by a json object and validated by a helper function instead of manually

@stale stale bot removed the stale label May 27, 2020
@dcodeIO dcodeIO changed the title asconfig.json - Working Issue RFC Support asconfig.json in addition to CLI arguments May 28, 2020
@MaxGraey
Copy link
Member

MaxGraey commented Aug 8, 2020

@jtenner Do we need keep opened this issue?

@jtenner jtenner closed this as completed Aug 8, 2020
@jtenner
Copy link
Contributor Author

jtenner commented Aug 8, 2020

No. I think our needs are covered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants