Skip to content

Merge oldest parent first #1404

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

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 37 additions & 28 deletions cli/asc.js
Original file line number Diff line number Diff line change
Expand Up @@ -280,11 +280,30 @@ exports.main = function main(argv, options, callback) {
const seenAsconfig = new Set();
seenAsconfig.add(path.join(baseDir, args.config));

while (asconfig) {
// merge target first, then merge options, then merge extended asconfigs
if (asconfig.targets && asconfig.targets[target]) {
args = optionsUtil.merge(exports.options, asconfig.targets[target], args);
// Keep array of configs
const configs = [asconfig];

// First find all parent configs and add them each to the front
while (asconfig && asconfig.extends) {
asconfigDir = path.isAbsolute(asconfig.extends)
// absolute extension path means we know the exact directory and location
? path.dirname(asconfig.extends)
// relative means we need to calculate a relative asconfigDir
: path.join(asconfigDir, path.dirname(asconfig.extends));
const fileName = path.basename(asconfig.extends);
const filePath = path.join(asconfigDir, fileName);
if (seenAsconfig.has(filePath)) {
asconfig = null;
} else {
seenAsconfig.add(filePath);
asconfig = getAsconfig(fileName, asconfigDir, readFile);
configs.unshift(asconfig);
}
}
let entries = new Set();
asconfig = configs.shift();
while (asconfig) {
// First merge options as the targets can overwrite them later
if (asconfig.options) {
if (asconfig.options.transform) {
// ensure that a transform's path is relative to the current config
Expand All @@ -298,43 +317,33 @@ exports.main = function main(argv, options, callback) {
return p;
});
}
args = optionsUtil.merge(exports.options, args, asconfig.options);
args = optionsUtil.merge(exports.options, asconfig.options, args);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the wrong order. The asconfig options need to be overridden by cli args.

The options.merge function puts the parent as the third parameter, preferring the second argument.

Did I miss something or mis-implement this algorithm?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. Just added more tests and sure enough it ignores the CLI args. The issue was that the defaults for the args was being set before the config properties were being added. So if you treat args as the current config it will prioritize the defaults. I just changed it to not add the defaults right away so we can get just the args passed and things seem to work.

}

// merge targets
if (asconfig.targets && asconfig.targets[target]) {
args = optionsUtil.merge(exports.options, asconfig.targets[target], args);
}

// entries are added to the compilation
if (asconfig.entries) {
for (const entry of asconfig.entries) {
argv.push(
path.isAbsolute(entry)
entries.add(
path.relative(baseDir, path.isAbsolute(entry)
? entry
// the entry is relative to the asconfig directory
: path.join(asconfigDir, entry)
);
}
}

// asconfig "extends" another config, merging options of it's parent
if (asconfig.extends) {
asconfigDir = path.isAbsolute(asconfig.extends)
// absolute extension path means we know the exact directory and location
? path.dirname(asconfig.extends)
// relative means we need to calculate a relative asconfigDir
: path.join(asconfigDir, path.dirname(asconfig.extends));
const fileName = path.basename(asconfig.extends);
const filePath = path.join(asconfigDir, fileName);
if (seenAsconfig.has(filePath)) {
asconfig = null;
} else {
seenAsconfig.add(filePath);
asconfig = getAsconfig(fileName, asconfigDir, readFile);
));
}
} else {
asconfig = null; // finished resolving the configuration chain
}
asconfig = configs.shift();
}

// Add entries
argv = argv.concat(Array.from(entries));

// If showConfig print args and exit
if (args.showConfig) {
args.argv = argv;
stderr.write(JSON.stringify(args, null, 2));
return callback(null);
}
Expand Down
2 changes: 1 addition & 1 deletion tests/asconfig/complicated/assembly/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@ assert(ASC_SHRINK_LEVEL == 1);
assert(ASC_FEATURE_SIMD);
let size = memory.size();
trace("size", 1, size);
assert(size == 30);
assert(size == 30, "expected 30 got " + size.toString());
4 changes: 3 additions & 1 deletion tests/asconfig/extends/asconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
}
},
"options": {
"enable": ["simd"]
"enable": ["simd"],
"runtime": "half",
"noEmit": false
},
"extends": "./extends.json"
}
6 changes: 6 additions & 0 deletions tests/asconfig/extends/expected.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"runtime": "half",
"noEmit": false,
"noAssert": true,
"enable": ["simd"]
}
4 changes: 3 additions & 1 deletion tests/asconfig/extends/extends.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
}
},
"options": {
"disable": ["simd"]
"disable": ["simd"],
"noEmit": true,
"noAssert": true
}
}
2 changes: 1 addition & 1 deletion tests/asconfig/extends/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"private": true,
"scripts": {
"test": "node ../index.js"
"test": "node ../index.js --showConfig && node ../index.js"
}
}
43 changes: 43 additions & 0 deletions tests/asconfig/index.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
const asc = require("../../cli/asc");
const loader = require("../../lib/loader");
const args = process.argv.slice(2);
const path = require('path');
const fs = require("fs");

/** @type {string} */
let stderr;
/** @type {Uint8Array} */
let binary;
asc.main(["assembly/index.ts", "--outFile", "output.wasm", "--explicitStart", ...args], {
Expand All @@ -11,13 +15,51 @@ asc.main(["assembly/index.ts", "--outFile", "output.wasm", "--explicitStart", ..
} else if (name !== "output.wasm.map") {
throw Error("Unexpected output file: " + name);
}
},
stderr: {
write(s) {
stderr = s;
}
}

}, (err) => {
if (err) {
console.error(err);
console.error(stderr);
process.exit(1);
}

const jsonPath = path.join(process.cwd(), "expected.json");
if (fs.existsSync(jsonPath) && stderr) {
const actual = JSON.parse(stderr);
const expected = require(jsonPath);
let errored = false;
for (let name of Object.getOwnPropertyNames(expected)) {
if (actual[name] !== expected[name]) {
// If object check just first level
if (typeof actual[name] === 'object' && typeof expected[name] === 'object') {
let error = false;
for (let field of Object.getOwnPropertyNames(actual[name])) {
if (actual[name][field] !== expected[name][field]) {
error = true;
break;
}
}
if (!error) {
continue;
}
}
console.error(name + ": " + actual[name] + " expected " + expected[name]);
errored = true;
}
}
if (errored) {
process.exit(1);
}
process.exit(0);
}


if (!binary) {
console.error("No binary was generated for the asconfig test in " + process.cwd());
process.exit(1);
Expand All @@ -29,6 +71,7 @@ asc.main(["assembly/index.ts", "--outFile", "output.wasm", "--explicitStart", ..
theModule.exports._start();
} catch (err) {
console.error("The wasm module _start() function failed in " + process.cwd());
console.error(err);
process.exit(1);
}
return 0;
Expand Down
5 changes: 3 additions & 2 deletions tests/asconfig/package.json
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
{
"private": true,
"scripts": {
"test": "npm run test:use-consts && npm run test:target && npm run test:entry-points && npm run test:complicated",
"test": "npm run test:use-consts && npm run test:target && npm run test:entry-points && npm run test:complicated && npm run test:extends",
"test:use-consts": "cd use-consts && npm run test",
"test:entry-points": "cd entry-points && npm run test",
"test:respect-inheritence": "cd respect-inheritence && npm run test",
"test:target": "cd target && npm run test",
"test:cyclical": "cd cyclical && npm run test",
"test:complicated": "cd complicated && npm run test"
"test:complicated": "cd complicated && npm run test",
"test:extends": "cd extends && npm run test"
}
}