From b8162e8075c5ade989ea353fe5310b3700ed4c4b Mon Sep 17 00:00:00 2001 From: Nikolas Grottendieck Date: Wed, 20 Sep 2023 00:21:55 +0200 Subject: [PATCH] fix: Maven Toolchains grows unexpectedly On self-hosted runners toolchains.xml may survive multiple runs and unexpectedly grow as a result of the toolchains setup simply appending the JDK definition even if one with the same `type` and `provides.id` already exists. Restructuring the parsing step and filtering the potentially existing list of toolchain definitions prevents this and also fixes toolchain.xml files that already contain duplicates. Fixes #530 --- __tests__/toolchains.test.ts | 451 ++++++++++++++++++++++++++++++++++- dist/setup/index.js | 79 +++--- src/toolchains.ts | 105 +++++--- 3 files changed, 561 insertions(+), 74 deletions(-) diff --git a/__tests__/toolchains.test.ts b/__tests__/toolchains.test.ts index 483077dc1..4aa1c1874 100644 --- a/__tests__/toolchains.test.ts +++ b/__tests__/toolchains.test.ts @@ -143,18 +143,467 @@ describe('toolchains tests', () => { `; const result = ` - + + + jdk + + 17 + Eclipse Temurin + temurin_17 + + + /opt/hostedtoolcache/Java_Temurin-Hotspot_jdk/17.0.1-12/x64 + + + + jdk + + 1.6 + Sun + sun_1.6 + + + /opt/jdk/sun/1.6 + + +`; + + fs.mkdirSync(m2Dir, {recursive: true}); + fs.writeFileSync(toolchainsFile, originalFile); + expect(fs.existsSync(m2Dir)).toBe(true); + expect(fs.existsSync(toolchainsFile)).toBe(true); + + await toolchains.createToolchainsSettings({ + jdkInfo, + settingsDirectory: m2Dir, + overwriteSettings: true + }); + + expect(fs.existsSync(m2Dir)).toBe(true); + expect(fs.existsSync(toolchainsFile)).toBe(true); + expect(fs.readFileSync(toolchainsFile, 'utf-8')).toEqual( + toolchains.generateToolchainDefinition( + originalFile, + jdkInfo.version, + jdkInfo.vendor, + jdkInfo.id, + jdkInfo.jdkHome + ) + ); + expect( + toolchains.generateToolchainDefinition( + originalFile, + jdkInfo.version, + jdkInfo.vendor, + jdkInfo.id, + jdkInfo.jdkHome + ) + ).toEqual(result); + }, 100000); + + it('does not discard custom elements in existing toolchain definitions', async () => { + const jdkInfo = { + version: '17', + vendor: 'Eclipse Temurin', + id: 'temurin_17', + jdkHome: '/opt/hostedtoolcache/Java_Temurin-Hotspot_jdk/17.0.1-12/x64' + }; + + const originalFile = ` + + jdk + + 1.6 + Sun + sun_1.6 + foo + + + /opt/jdk/sun/1.6 + /usr/local/bin/bash + + + `; + const result = ` + + + jdk + + 17 + Eclipse Temurin + temurin_17 + + + /opt/hostedtoolcache/Java_Temurin-Hotspot_jdk/17.0.1-12/x64 + + jdk 1.6 Sun sun_1.6 + foo /opt/jdk/sun/1.6 + /usr/local/bin/bash +`; + + fs.mkdirSync(m2Dir, {recursive: true}); + fs.writeFileSync(toolchainsFile, originalFile); + expect(fs.existsSync(m2Dir)).toBe(true); + expect(fs.existsSync(toolchainsFile)).toBe(true); + + await toolchains.createToolchainsSettings({ + jdkInfo, + settingsDirectory: m2Dir, + overwriteSettings: true + }); + + expect(fs.existsSync(m2Dir)).toBe(true); + expect(fs.existsSync(toolchainsFile)).toBe(true); + expect(fs.readFileSync(toolchainsFile, 'utf-8')).toEqual( + toolchains.generateToolchainDefinition( + originalFile, + jdkInfo.version, + jdkInfo.vendor, + jdkInfo.id, + jdkInfo.jdkHome + ) + ); + expect( + toolchains.generateToolchainDefinition( + originalFile, + jdkInfo.version, + jdkInfo.vendor, + jdkInfo.id, + jdkInfo.jdkHome + ) + ).toEqual(result); + }, 100000); + + it('does not discard existing, custom toolchain definitions', async () => { + const jdkInfo = { + version: '17', + vendor: 'Eclipse Temurin', + id: 'temurin_17', + jdkHome: '/opt/hostedtoolcache/Java_Temurin-Hotspot_jdk/17.0.1-12/x64' + }; + + const originalFile = ` + + foo + + baz + + + /usr/local/bin/foo + + + `; + const result = ` + + + jdk + + 17 + Eclipse Temurin + temurin_17 + + + /opt/hostedtoolcache/Java_Temurin-Hotspot_jdk/17.0.1-12/x64 + + + + foo + + baz + + + /usr/local/bin/foo + + +`; + + fs.mkdirSync(m2Dir, {recursive: true}); + fs.writeFileSync(toolchainsFile, originalFile); + expect(fs.existsSync(m2Dir)).toBe(true); + expect(fs.existsSync(toolchainsFile)).toBe(true); + + await toolchains.createToolchainsSettings({ + jdkInfo, + settingsDirectory: m2Dir, + overwriteSettings: true + }); + + expect(fs.existsSync(m2Dir)).toBe(true); + expect(fs.existsSync(toolchainsFile)).toBe(true); + expect(fs.readFileSync(toolchainsFile, 'utf-8')).toEqual( + toolchains.generateToolchainDefinition( + originalFile, + jdkInfo.version, + jdkInfo.vendor, + jdkInfo.id, + jdkInfo.jdkHome + ) + ); + expect( + toolchains.generateToolchainDefinition( + originalFile, + jdkInfo.version, + jdkInfo.vendor, + jdkInfo.id, + jdkInfo.jdkHome + ) + ).toEqual(result); + }, 100000); + + it('does not duplicate existing toolchain definitions', async () => { + const jdkInfo = { + version: '17', + vendor: 'Eclipse Temurin', + id: 'temurin_17', + jdkHome: '/opt/hostedtoolcache/Java_Temurin-Hotspot_jdk/17.0.1-12/x64' + }; + + const originalFile = ` + + jdk + + 17 + Eclipse Temurin + temurin_17 + + + /opt/hostedtoolcache/Java_Temurin-Hotspot_jdk/17.0.1-12/x64 + + + `; + const result = ` + + + jdk + + 17 + Eclipse Temurin + temurin_17 + + + /opt/hostedtoolcache/Java_Temurin-Hotspot_jdk/17.0.1-12/x64 + + +`; + + fs.mkdirSync(m2Dir, {recursive: true}); + fs.writeFileSync(toolchainsFile, originalFile); + expect(fs.existsSync(m2Dir)).toBe(true); + expect(fs.existsSync(toolchainsFile)).toBe(true); + + await toolchains.createToolchainsSettings({ + jdkInfo, + settingsDirectory: m2Dir, + overwriteSettings: true + }); + + expect(fs.existsSync(m2Dir)).toBe(true); + expect(fs.existsSync(toolchainsFile)).toBe(true); + expect(fs.readFileSync(toolchainsFile, 'utf-8')).toEqual( + toolchains.generateToolchainDefinition( + originalFile, + jdkInfo.version, + jdkInfo.vendor, + jdkInfo.id, + jdkInfo.jdkHome + ) + ); + expect( + toolchains.generateToolchainDefinition( + originalFile, + jdkInfo.version, + jdkInfo.vendor, + jdkInfo.id, + jdkInfo.jdkHome + ) + ).toEqual(result); + }, 100000); + + it('does not duplicate existing toolchain definitions if multiple exist', async () => { + const jdkInfo = { + version: '17', + vendor: 'Eclipse Temurin', + id: 'temurin_17', + jdkHome: '/opt/hostedtoolcache/Java_Temurin-Hotspot_jdk/17.0.1-12/x64' + }; + + const originalFile = ` + + jdk + + 1.6 + Sun + sun_1.6 + + + /opt/jdk/sun/1.6 + + + + jdk + + 17 + Eclipse Temurin + temurin_17 + + + /opt/hostedtoolcache/Java_Temurin-Hotspot_jdk/17.0.1-12/x64 + + + `; + const result = ` + + + jdk + + 17 + Eclipse Temurin + temurin_17 + + + /opt/hostedtoolcache/Java_Temurin-Hotspot_jdk/17.0.1-12/x64 + + + + jdk + + 1.6 + Sun + sun_1.6 + + + /opt/jdk/sun/1.6 + + +`; + + fs.mkdirSync(m2Dir, {recursive: true}); + fs.writeFileSync(toolchainsFile, originalFile); + expect(fs.existsSync(m2Dir)).toBe(true); + expect(fs.existsSync(toolchainsFile)).toBe(true); + + await toolchains.createToolchainsSettings({ + jdkInfo, + settingsDirectory: m2Dir, + overwriteSettings: true + }); + + expect(fs.existsSync(m2Dir)).toBe(true); + expect(fs.existsSync(toolchainsFile)).toBe(true); + expect(fs.readFileSync(toolchainsFile, 'utf-8')).toEqual( + toolchains.generateToolchainDefinition( + originalFile, + jdkInfo.version, + jdkInfo.vendor, + jdkInfo.id, + jdkInfo.jdkHome + ) + ); + expect( + toolchains.generateToolchainDefinition( + originalFile, + jdkInfo.version, + jdkInfo.vendor, + jdkInfo.id, + jdkInfo.jdkHome + ) + ).toEqual(result); + }, 100000); + + it('handles an empty list of existing toolchains correctly', async () => { + const jdkInfo = { + version: '17', + vendor: 'Eclipse Temurin', + id: 'temurin_17', + jdkHome: '/opt/hostedtoolcache/Java_Temurin-Hotspot_jdk/17.0.1-12/x64' + }; + + const originalFile = ` + `; + const result = ` + + + jdk + + 17 + Eclipse Temurin + temurin_17 + + + /opt/hostedtoolcache/Java_Temurin-Hotspot_jdk/17.0.1-12/x64 + + +`; + + fs.mkdirSync(m2Dir, {recursive: true}); + fs.writeFileSync(toolchainsFile, originalFile); + expect(fs.existsSync(m2Dir)).toBe(true); + expect(fs.existsSync(toolchainsFile)).toBe(true); + + await toolchains.createToolchainsSettings({ + jdkInfo, + settingsDirectory: m2Dir, + overwriteSettings: true + }); + + expect(fs.existsSync(m2Dir)).toBe(true); + expect(fs.existsSync(toolchainsFile)).toBe(true); + expect(fs.readFileSync(toolchainsFile, 'utf-8')).toEqual( + toolchains.generateToolchainDefinition( + originalFile, + jdkInfo.version, + jdkInfo.vendor, + jdkInfo.id, + jdkInfo.jdkHome + ) + ); + expect( + toolchains.generateToolchainDefinition( + originalFile, + jdkInfo.version, + jdkInfo.vendor, + jdkInfo.id, + jdkInfo.jdkHome + ) + ).toEqual(result); + }, 100000); + + it('handles an empty existing toolchains.xml correctly', async () => { + const jdkInfo = { + version: '17', + vendor: 'Eclipse Temurin', + id: 'temurin_17', + jdkHome: '/opt/hostedtoolcache/Java_Temurin-Hotspot_jdk/17.0.1-12/x64' + }; + + const originalFile = ``; + const result = ` + jdk diff --git a/dist/setup/index.js b/dist/setup/index.js index 916f488bb..4995e0c2e 100644 --- a/dist/setup/index.js +++ b/dist/setup/index.js @@ -125016,46 +125016,53 @@ function createToolchainsSettings({ jdkInfo, settingsDirectory, overwriteSetting exports.createToolchainsSettings = createToolchainsSettings; // only exported for testing purposes function generateToolchainDefinition(original, version, vendor, id, jdkHome) { - let xmlObj; + let jsToolchains = [ + { + type: 'jdk', + provides: { + version: `${version}`, + vendor: `${vendor}`, + id: `${id}` + }, + configuration: { + jdkHome: `${jdkHome}` + } + } + ]; if (original === null || original === void 0 ? void 0 : original.length) { - xmlObj = (0, xmlbuilder2_1.create)(original) + // convert existing toolchains into TS native objects for better handling + // xmlbuilder2 will convert the document into a `{toolchains: { toolchain: [] | {} }}` structure + // instead of the desired `toolchains: [{}]` one or simply `[{}]` + const jsObj = (0, xmlbuilder2_1.create)(original) .root() - .ele({ - toolchain: { - type: 'jdk', - provides: { - version: `${version}`, - vendor: `${vendor}`, - id: `${id}` - }, - configuration: { - jdkHome: `${jdkHome}` - } + .toObject(); + if (jsObj.toolchains && jsObj.toolchains.toolchain) { + // in case only a single child exists xmlbuilder2 will not create an array and using verbose = true equally doesn't work here + // See https://oozcitak.github.io/xmlbuilder2/serialization.html#js-object-and-map-serializers for details + if (Array.isArray(jsObj.toolchains.toolchain)) { + jsToolchains.push(...jsObj.toolchains.toolchain); } - }); - } - else - xmlObj = (0, xmlbuilder2_1.create)({ - toolchains: { - '@xmlns': 'http://maven.apache.org/TOOLCHAINS/1.1.0', - '@xmlns:xsi': 'http://www.w3.org/2001/XMLSchema-instance', - '@xsi:schemaLocation': 'http://maven.apache.org/TOOLCHAINS/1.1.0 https://maven.apache.org/xsd/toolchains-1.1.0.xsd', - toolchain: [ - { - type: 'jdk', - provides: { - version: `${version}`, - vendor: `${vendor}`, - id: `${id}` - }, - configuration: { - jdkHome: `${jdkHome}` - } - } - ] + else { + jsToolchains.push(jsObj.toolchains.toolchain); } - }); - return xmlObj.end({ + } + // remove potential duplicates based on type & id (which should be a unique combination); + // self.findIndex will only return the first occurrence, ensuring duplicates are skipped + jsToolchains = jsToolchains.filter((value, index, self) => + // ensure non-jdk toolchains are kept in the results, we must not touch them because they belong to the user + value.type !== 'jdk' || + index === + self.findIndex(t => t.type === value.type && t.provides.id === value.provides.id)); + } + // TODO: technically bad because we shouldn't re-create the toolchains root node (with possibly different schema values) if it already exists, however, just overriding the toolchain array with xmlbuilder2 is … uh non-trivial + return (0, xmlbuilder2_1.create)({ + toolchains: { + '@xmlns': 'http://maven.apache.org/TOOLCHAINS/1.1.0', + '@xmlns:xsi': 'http://www.w3.org/2001/XMLSchema-instance', + '@xsi:schemaLocation': 'http://maven.apache.org/TOOLCHAINS/1.1.0 https://maven.apache.org/xsd/toolchains-1.1.0.xsd', + toolchain: jsToolchains + } + }).end({ format: 'xml', wellFormed: false, headless: false, diff --git a/src/toolchains.ts b/src/toolchains.ts index cbb667eb7..9d1ce487b 100644 --- a/src/toolchains.ts +++ b/src/toolchains.ts @@ -84,47 +84,59 @@ export function generateToolchainDefinition( id: string, jdkHome: string ) { - let xmlObj; + let jsToolchains: Toolchain[] = [ + { + type: 'jdk', + provides: { + version: `${version}`, + vendor: `${vendor}`, + id: `${id}` + }, + configuration: { + jdkHome: `${jdkHome}` + } + } + ]; if (original?.length) { - xmlObj = xmlCreate(original) + // convert existing toolchains into TS native objects for better handling + // xmlbuilder2 will convert the document into a `{toolchains: { toolchain: [] | {} }}` structure + // instead of the desired `toolchains: [{}]` one or simply `[{}]` + const jsObj = xmlCreate(original) .root() - .ele({ - toolchain: { - type: 'jdk', - provides: { - version: `${version}`, - vendor: `${vendor}`, - id: `${id}` - }, - configuration: { - jdkHome: `${jdkHome}` - } - } - }); - } else - xmlObj = xmlCreate({ - toolchains: { - '@xmlns': 'http://maven.apache.org/TOOLCHAINS/1.1.0', - '@xmlns:xsi': 'http://www.w3.org/2001/XMLSchema-instance', - '@xsi:schemaLocation': - 'http://maven.apache.org/TOOLCHAINS/1.1.0 https://maven.apache.org/xsd/toolchains-1.1.0.xsd', - toolchain: [ - { - type: 'jdk', - provides: { - version: `${version}`, - vendor: `${vendor}`, - id: `${id}` - }, - configuration: { - jdkHome: `${jdkHome}` - } - } - ] + .toObject() as unknown as ExtractedToolchains; + if (jsObj.toolchains && jsObj.toolchains.toolchain) { + // in case only a single child exists xmlbuilder2 will not create an array and using verbose = true equally doesn't work here + // See https://oozcitak.github.io/xmlbuilder2/serialization.html#js-object-and-map-serializers for details + if (Array.isArray(jsObj.toolchains.toolchain)) { + jsToolchains.push(...jsObj.toolchains.toolchain); + } else { + jsToolchains.push(jsObj.toolchains.toolchain); } - }); + } + + // remove potential duplicates based on type & id (which should be a unique combination); + // self.findIndex will only return the first occurrence, ensuring duplicates are skipped + jsToolchains = jsToolchains.filter( + (value, index, self) => + // ensure non-jdk toolchains are kept in the results, we must not touch them because they belong to the user + value.type !== 'jdk' || + index === + self.findIndex( + t => t.type === value.type && t.provides.id === value.provides.id + ) + ); + } - return xmlObj.end({ + // TODO: technically bad because we shouldn't re-create the toolchains root node (with possibly different schema values) if it already exists, however, just overriding the toolchain array with xmlbuilder2 is … uh non-trivial + return xmlCreate({ + toolchains: { + '@xmlns': 'http://maven.apache.org/TOOLCHAINS/1.1.0', + '@xmlns:xsi': 'http://www.w3.org/2001/XMLSchema-instance', + '@xsi:schemaLocation': + 'http://maven.apache.org/TOOLCHAINS/1.1.0 https://maven.apache.org/xsd/toolchains-1.1.0.xsd', + toolchain: jsToolchains + } + }).end({ format: 'xml', wellFormed: false, headless: false, @@ -167,3 +179,22 @@ async function writeToolchainsFileToDisk( flag: 'w' }); } + +interface ExtractedToolchains { + toolchains: { + toolchain: Toolchain[] | Toolchain; + }; +} + +// Toolchain type definition according to Maven Toolchains XSD 1.1.0 +interface Toolchain { + type: string; + provides: + | { + version: string; + vendor: string; + id: string; + } + | any; + configuration: any; +}