From 66d5dc10b1c2011db7357d807279031cd2cc76ef Mon Sep 17 00:00:00 2001 From: Luis Soares Date: Wed, 20 Sep 2023 17:34:35 +0100 Subject: [PATCH 1/7] Rework loadAllFilesAST and getManifest --- src/Program.ts | 76 ++++++++++++++++++++++++------------------- src/ProgramBuilder.ts | 59 ++++++++++++--------------------- 2 files changed, 62 insertions(+), 73 deletions(-) diff --git a/src/Program.ts b/src/Program.ts index 066b17285..1bb45c50e 100644 --- a/src/Program.ts +++ b/src/Program.ts @@ -1349,51 +1349,59 @@ export class Program { return files; } + private _manifest: Map; + /** - * Get a map of the manifest information + * Try to find and load the manifest into memory + * @param staticFiles A list of all static files */ - public getManifest() { - if (!this._manifest) { - //load the manifest file. - //TODO update this to get the manifest from the files array or require it in the options...we shouldn't assume the location of the manifest - let manifestPath = path.join(this.options.rootDir, 'manifest'); - - let contents: string; - try { - //we only load this manifest once, so do it sync to improve speed downstream - contents = fsExtra.readFileSync(manifestPath, 'utf-8'); - let parsedManifest = parseManifest(contents); - - // Lift the bs_consts defined in the manifest - let bsConsts = getBsConst(parsedManifest, false); - - // Override or delete any bs_consts defined in the bs config - for (const key in this.options?.manifest?.bs_const) { - const value = this.options.manifest.bs_const[key]; - if (value === null) { - bsConsts.delete(key); - } else { - bsConsts.set(key, value); - } + public loadManifest(staticFiles?: FileObj[]) { + let manifestPath = path.join(this.options.rootDir, 'manifest'); // @todo + + try { + // we only load this manifest once, so do it sync to improve speed downstream + const contents = fsExtra.readFileSync(manifestPath, 'utf-8'); + const parsedManifest = parseManifest(contents); + + // Lift the bs_consts defined in the manifest + let bsConsts = getBsConst(parsedManifest, false); + + // Override or delete any bs_consts defined in the bs config + for (const key in this.options?.manifest?.bs_const) { + const value = this.options.manifest.bs_const[key]; + if (value === null) { + bsConsts.delete(key); + } else { + bsConsts.set(key, value); } + } - // convert the new list of bs consts back into a string for the rest of the down stream systems to use - let constString = ''; - for (const [key, value] of bsConsts) { - constString += `${constString !== '' ? ';' : ''}${key}=${value.toString()}`; - } + // convert the new list of bs consts back into a string for the rest of the down stream systems to use + let constString = ''; + for (const [key, value] of bsConsts) { + constString += `${constString !== '' ? ';' : ''}${key}=${value.toString()}`; + } - // Set the updated bs_const value - parsedManifest.set('bs_const', constString); + // Set the updated bs_const value + parsedManifest.set('bs_const', constString); - this._manifest = parsedManifest; - } catch (err) { + this._manifest = parsedManifest; + } finally { + if (!this._manifest) { this._manifest = new Map(); } } + } + + /** + * Get a map of the manifest information + */ + public getManifest() { + if (!this._manifest) { + this.loadManifest(); + } return this._manifest; } - private _manifest: Map; public dispose() { this.plugins.emit('beforeProgramDispose', { program: this }); diff --git a/src/ProgramBuilder.ts b/src/ProgramBuilder.ts index 434cb5ad9..de82d8434 100644 --- a/src/ProgramBuilder.ts +++ b/src/ProgramBuilder.ts @@ -466,59 +466,40 @@ export class ProgramBuilder { */ private async loadAllFilesAST() { await this.logger.time(LogLevel.log, ['Parsing files'], async () => { - let errorCount = 0; let files = await this.logger.time(LogLevel.debug, ['getFilePaths'], async () => { return util.getFilePaths(this.options); }); this.logger.trace('ProgramBuilder.loadAllFilesAST() files:', files); + const acceptableExtensions = ['.bs', '.brs', '.xml']; const typedefFiles = [] as FileObj[]; - const nonTypedefFiles = [] as FileObj[]; + const staticFiles = [] as FileObj[]; + const sourceFiles = [] as FileObj[]; for (const file of files) { const srcLower = file.src.toLowerCase(); if (srcLower.endsWith('.d.bs')) { typedefFiles.push(file); + } else if (acceptableExtensions.includes(path.extname(file.src).toLowerCase())) { + sourceFiles.push(file); } else { - nonTypedefFiles.push(file); + staticFiles.push(file); } } - //preload every type definition file first, which eliminates duplicate file loading - await Promise.all( - typedefFiles.map(async (fileObj) => { - try { - this.program.setFile( - fileObj, - await this.getFileContents(fileObj.src) - ); - } catch (e) { - //log the error, but don't fail this process because the file might be fixable later - this.logger.log(e); - } - }) - ); - - const acceptableExtensions = ['.bs', '.brs', '.xml']; - //parse every file other than the type definitions - await Promise.all( - nonTypedefFiles.map(async (fileObj) => { - try { - let fileExtension = path.extname(fileObj.src).toLowerCase(); - - //only process certain file types - if (acceptableExtensions.includes(fileExtension)) { - this.program.setFile( - fileObj, - await this.getFileContents(fileObj.src) - ); - } - } catch (e) { - //log the error, but don't fail this process because the file might be fixable later - this.logger.log(e); - } - }) - ); - return errorCount; + const loadFile = async (fileObj) => { + try { + this.program.setFile( + fileObj, + await this.getFileContents(fileObj.src) + ); + } catch (e) { + // log the error, but don't fail this process because the file might be fixable later + this.logger.log(e); + } + }; + await Promise.all(typedefFiles.map(loadFile)); // first, preload every type definition file, which eliminates duplicate file loading + await Promise.all(staticFiles.map(loadFile)); // then, preload all static files + await Promise.all(sourceFiles.map(loadFile)); // finally, parse source files }); } From 5b899056cfe8cdeb8fa55f507d20743aee1d3988 Mon Sep 17 00:00:00 2001 From: Luis Soares Date: Wed, 20 Sep 2023 17:34:49 +0100 Subject: [PATCH 2/7] Set builder to load manifest before all other files --- src/ProgramBuilder.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/ProgramBuilder.ts b/src/ProgramBuilder.ts index de82d8434..5a6433288 100644 --- a/src/ProgramBuilder.ts +++ b/src/ProgramBuilder.ts @@ -486,6 +486,8 @@ export class ProgramBuilder { } } + this.program.loadManifest(staticFiles); + const loadFile = async (fileObj) => { try { this.program.setFile( From 361ccb762c48c47f85a0bf46f96a4831702f5a05 Mon Sep 17 00:00:00 2001 From: Luis Soares Date: Fri, 22 Sep 2023 12:20:27 +0100 Subject: [PATCH 3/7] Tweak mechanism to accept one manifest entry --- src/Program.ts | 8 +++++--- src/ProgramBuilder.ts | 12 +++++++++--- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/src/Program.ts b/src/Program.ts index 1bb45c50e..aa23d367a 100644 --- a/src/Program.ts +++ b/src/Program.ts @@ -1353,10 +1353,12 @@ export class Program { /** * Try to find and load the manifest into memory - * @param staticFiles A list of all static files + * @param manifestFileObj A pointer to a potential manifest file object found during loading */ - public loadManifest(staticFiles?: FileObj[]) { - let manifestPath = path.join(this.options.rootDir, 'manifest'); // @todo + public loadManifest(manifestFileObj?: FileObj) { + let manifestPath = manifestFileObj + ? manifestFileObj.src + : path.join(this.options.rootDir, 'manifest'); try { // we only load this manifest once, so do it sync to improve speed downstream diff --git a/src/ProgramBuilder.ts b/src/ProgramBuilder.ts index 5a6433288..fb3c3a9e4 100644 --- a/src/ProgramBuilder.ts +++ b/src/ProgramBuilder.ts @@ -471,22 +471,28 @@ export class ProgramBuilder { }); this.logger.trace('ProgramBuilder.loadAllFilesAST() files:', files); - const acceptableExtensions = ['.bs', '.brs', '.xml']; + const acceptableSourceExtensions = ['.bs', '.brs', '.xml']; const typedefFiles = [] as FileObj[]; const staticFiles = [] as FileObj[]; const sourceFiles = [] as FileObj[]; + let manifestFile: FileObj | null = null; for (const file of files) { const srcLower = file.src.toLowerCase(); if (srcLower.endsWith('.d.bs')) { typedefFiles.push(file); - } else if (acceptableExtensions.includes(path.extname(file.src).toLowerCase())) { + } else if (acceptableSourceExtensions.includes(path.extname(file.src).toLowerCase())) { sourceFiles.push(file); } else { staticFiles.push(file); + if (srcLower.endsWith('/manifest')) { + manifestFile = file; + } } } - this.program.loadManifest(staticFiles); + if (manifestFile) { + this.program.loadManifest(manifestFile); + } const loadFile = async (fileObj) => { try { From a7935a1a3dcfd1d71a634cd95d1dc063f75414c0 Mon Sep 17 00:00:00 2001 From: Luis Soares Date: Fri, 22 Sep 2023 13:05:53 +0100 Subject: [PATCH 4/7] Avoid loading static files, tweak signature --- src/ProgramBuilder.ts | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/ProgramBuilder.ts b/src/ProgramBuilder.ts index fb3c3a9e4..c48d7a6ec 100644 --- a/src/ProgramBuilder.ts +++ b/src/ProgramBuilder.ts @@ -473,18 +473,17 @@ export class ProgramBuilder { const acceptableSourceExtensions = ['.bs', '.brs', '.xml']; const typedefFiles = [] as FileObj[]; - const staticFiles = [] as FileObj[]; const sourceFiles = [] as FileObj[]; let manifestFile: FileObj | null = null; + for (const file of files) { const srcLower = file.src.toLowerCase(); if (srcLower.endsWith('.d.bs')) { typedefFiles.push(file); - } else if (acceptableSourceExtensions.includes(path.extname(file.src).toLowerCase())) { + } else if (acceptableSourceExtensions.includes(path.extname(srcLower))) { sourceFiles.push(file); } else { - staticFiles.push(file); - if (srcLower.endsWith('/manifest')) { + if (file.dest.toLowerCase() === 'manifest') { // todo: evaluate if this is acceptable. manifestFile = file; } } @@ -496,18 +495,13 @@ export class ProgramBuilder { const loadFile = async (fileObj) => { try { - this.program.setFile( - fileObj, - await this.getFileContents(fileObj.src) - ); + this.program.setFile(fileObj, await this.getFileContents(fileObj.src)); } catch (e) { - // log the error, but don't fail this process because the file might be fixable later - this.logger.log(e); + this.logger.log(e); // log the error, but don't fail this process because the file might be fixable later } }; - await Promise.all(typedefFiles.map(loadFile)); // first, preload every type definition file, which eliminates duplicate file loading - await Promise.all(staticFiles.map(loadFile)); // then, preload all static files - await Promise.all(sourceFiles.map(loadFile)); // finally, parse source files + await Promise.all(typedefFiles.map(loadFile)); // preload every type definition file, which eliminates duplicate file loading + await Promise.all(sourceFiles.map(loadFile)); // parse source files }); } From e595aadcf5102e7ad8ecaf9f7f6ebc5e2ca94e72 Mon Sep 17 00:00:00 2001 From: Luis Soares Date: Mon, 6 Nov 2023 12:07:07 +0000 Subject: [PATCH 5/7] Fix test crashes due to uncaught exception --- src/Program.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Program.ts b/src/Program.ts index aa23d367a..0dd360fd2 100644 --- a/src/Program.ts +++ b/src/Program.ts @@ -1388,7 +1388,7 @@ export class Program { parsedManifest.set('bs_const', constString); this._manifest = parsedManifest; - } finally { + } catch (e) { if (!this._manifest) { this._manifest = new Map(); } From 63edcb2fe263e143225bfc718b46c70c91fd93de Mon Sep 17 00:00:00 2001 From: Luis Soares Date: Mon, 6 Nov 2023 15:00:34 +0000 Subject: [PATCH 6/7] Adjust program tests --- src/Program.spec.ts | 25 +++++++++++++++++-- src/Program.ts | 56 +++++++++++++++++++++++-------------------- src/ProgramBuilder.ts | 2 +- 3 files changed, 54 insertions(+), 29 deletions(-) diff --git a/src/Program.spec.ts b/src/Program.spec.ts index 5367495cf..04b405980 100644 --- a/src/Program.spec.ts +++ b/src/Program.spec.ts @@ -2967,7 +2967,7 @@ describe('Program', () => { }); }); - describe('getManifest', () => { + describe('manifest', () => { beforeEach(() => { fsExtra.emptyDirSync(tempDir); fsExtra.writeFileSync(`${tempDir}/manifest`, trim` @@ -2989,7 +2989,28 @@ describe('Program', () => { program.dispose(); }); - it('loads the manifest', () => { + it('loads the manifest from project root', () => { + let manifest = program.getManifest(); + testCommonManifestValues(manifest); + expect(manifest.get('bs_const')).to.equal('DEBUG=false'); + }); + + it('loads the manifest from a FileObj', () => { + fsExtra.emptyDirSync(tempDir); + fsExtra.ensureDirSync(`${tempDir}/someDeepDir`); + fsExtra.writeFileSync(`${tempDir}/someDeepDir/manifest`, trim` + # Channel Details + title=sample manifest + major_version=2 + minor_version=0 + build_version=0 + supports_input_launch=1 + bs_const=DEBUG=false + `); + program.loadManifest({ + src: `${tempDir}/someDeepDir/manifest`, + dest: 'manifest' + }); let manifest = program.getManifest(); testCommonManifestValues(manifest); expect(manifest.get('bs_const')).to.equal('DEBUG=false'); diff --git a/src/Program.ts b/src/Program.ts index 0dd360fd2..27022543f 100644 --- a/src/Program.ts +++ b/src/Program.ts @@ -1351,6 +1351,34 @@ export class Program { private _manifest: Map; + /** + * Modify a parsed manifest map by reading `bs_const` and injecting values from `options.manifest.bs_const` + * @param parsedManifest The manifest map to read from and modify + */ + private buildBsConstsIntoParsedManifest(parsedManifest: Map) { + // Lift the bs_consts defined in the manifest + let bsConsts = getBsConst(parsedManifest, false); + + // Override or delete any bs_consts defined in the bs config + for (const key in this.options?.manifest?.bs_const) { + const value = this.options.manifest.bs_const[key]; + if (value === null) { + bsConsts.delete(key); + } else { + bsConsts.set(key, value); + } + } + + // convert the new list of bs consts back into a string for the rest of the down stream systems to use + let constString = ''; + for (const [key, value] of bsConsts) { + constString += `${constString !== '' ? ';' : ''}${key}=${value.toString()}`; + } + + // Set the updated bs_const value + parsedManifest.set('bs_const', constString); + } + /** * Try to find and load the manifest into memory * @param manifestFileObj A pointer to a potential manifest file object found during loading @@ -1364,34 +1392,10 @@ export class Program { // we only load this manifest once, so do it sync to improve speed downstream const contents = fsExtra.readFileSync(manifestPath, 'utf-8'); const parsedManifest = parseManifest(contents); - - // Lift the bs_consts defined in the manifest - let bsConsts = getBsConst(parsedManifest, false); - - // Override or delete any bs_consts defined in the bs config - for (const key in this.options?.manifest?.bs_const) { - const value = this.options.manifest.bs_const[key]; - if (value === null) { - bsConsts.delete(key); - } else { - bsConsts.set(key, value); - } - } - - // convert the new list of bs consts back into a string for the rest of the down stream systems to use - let constString = ''; - for (const [key, value] of bsConsts) { - constString += `${constString !== '' ? ';' : ''}${key}=${value.toString()}`; - } - - // Set the updated bs_const value - parsedManifest.set('bs_const', constString); - + this.buildBsConstsIntoParsedManifest(parsedManifest); this._manifest = parsedManifest; } catch (e) { - if (!this._manifest) { - this._manifest = new Map(); - } + this._manifest = new Map(); } } diff --git a/src/ProgramBuilder.ts b/src/ProgramBuilder.ts index c48d7a6ec..946f9e9da 100644 --- a/src/ProgramBuilder.ts +++ b/src/ProgramBuilder.ts @@ -483,7 +483,7 @@ export class ProgramBuilder { } else if (acceptableSourceExtensions.includes(path.extname(srcLower))) { sourceFiles.push(file); } else { - if (file.dest.toLowerCase() === 'manifest') { // todo: evaluate if this is acceptable. + if (file.dest.toLowerCase() === 'manifest') { manifestFile = file; } } From ee356191f929fc62ac53064cf2bf15b840218acd Mon Sep 17 00:00:00 2001 From: Luis Soares Date: Tue, 7 Nov 2023 11:27:51 +0000 Subject: [PATCH 7/7] Add test for load manifest order --- src/ProgramBuilder.spec.ts | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/ProgramBuilder.spec.ts b/src/ProgramBuilder.spec.ts index f6a72c712..9f2403411 100644 --- a/src/ProgramBuilder.spec.ts +++ b/src/ProgramBuilder.spec.ts @@ -60,6 +60,28 @@ describe('ProgramBuilder', () => { expect(stub.getCalls()).to.be.lengthOf(3); }); + it('finds and loads a manifest before all other files', async () => { + sinon.stub(util, 'getFilePaths').returns(Promise.resolve([{ + src: 'file1.brs', + dest: 'file1.brs' + }, { + src: 'file2.bs', + dest: 'file2.bs' + }, { + src: 'file3.xml', + dest: 'file4.xml' + }, { + src: 'manifest', + dest: 'manifest' + }])); + + let stubLoadManifest = sinon.stub(builder.program, 'loadManifest'); + let stubSetFile = sinon.stub(builder.program, 'setFile'); + sinon.stub(builder, 'getFileContents').returns(Promise.resolve('')); + await builder['loadAllFilesAST'](); + expect(stubLoadManifest.calledBefore(stubSetFile)).to.be.true; + }); + it('loads all type definitions first', async () => { const requestedFiles = [] as string[]; builder['fileResolvers'].push((filePath) => {