Skip to content

Commit

Permalink
[FIX] ProjectBuilder now can be executed in parallel (#669)
Browse files Browse the repository at this point in the history 
Fixes: SAP/ui5-tooling#887

ProjectBuilder was using a singleton of the logger module. Basically,
the logger was constructed only once, during module evaluation, but used
throughout the whole module.
The tricky part is that the `ProjectBuilder`'s logger has this
`setProjects()` method that acts as an allow list of the projects being
built. As the logger used to be a singleton within this module/class and
the async behaviour of the whole module, calling `setProject()` was
polluting the internal `projects` property.

With this fix, every `ProjectBuilder` instance has its own`BuildLogger`
instance that solves the issue.
  • Loading branch information
@d3xter666
d3xter666 authored Oct 20, 2023
1 parent 052c20b commit f652461
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 26 deletions.
53 changes: 27 additions & 26 deletions lib/build/ProjectBuilder.js
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import {rimraf} from "rimraf";
import * as resourceFactory from "@ui5/fs/resourceFactory";
import BuildLogger from "@ui5/logger/internal/loggers/Build";
const log = new BuildLogger("ProjectBuilder");
import composeProjectList from "./helpers/composeProjectList.js";
import BuildContext from "./helpers/BuildContext.js";
import prettyHrtime from "pretty-hrtime";
Expand All @@ -12,6 +11,7 @@ import prettyHrtime from "pretty-hrtime";
* @alias @ui5/project/build/ProjectBuilder
*/
class ProjectBuilder {
#log;
/**
* Build Configuration
*
Expand Down Expand Up @@ -112,6 +112,7 @@ class ProjectBuilder {

this._graph = graph;
this._buildContext = new BuildContext(graph, taskRepository, buildConfig);
this.#log = new BuildLogger("ProjectBuilder");
}

/**
Expand Down Expand Up @@ -148,8 +149,8 @@ class ProjectBuilder {
}
}
const rootProjectName = this._graph.getRoot().getName();
log.info(`Preparing build for project ${rootProjectName}`);
log.info(` Target directory: ${destPath}`);
this.#log.info(`Preparing build for project ${rootProjectName}`);
this.#log.info(` Target directory: ${destPath}`);

// Get project filter function based on include/exclude params
// (also logs some info to console)
Expand Down Expand Up @@ -195,19 +196,19 @@ class ProjectBuilder {
}
});

log.setProjects(queue.map((projectBuildContext) => {
this.#log.setProjects(queue.map((projectBuildContext) => {
return projectBuildContext.getProject().getName();
}));
if (queue.length > 1) { // Do not log if only the root project is being built
log.info(`Processing ${queue.length} projects`);
this.#log.info(`Processing ${queue.length} projects`);
if (alreadyBuilt.length) {
log.info(` Reusing build results of ${alreadyBuilt.length} projects`);
log.info(` Building ${queue.length - alreadyBuilt.length} projects`);
this.#log.info(` Reusing build results of ${alreadyBuilt.length} projects`);
this.#log.info(` Building ${queue.length - alreadyBuilt.length} projects`);
}

if (log.isLevelEnabled("verbose")) {
log.verbose(` Required projects:`);
log.verbose(` ${queue
if (this.#log.isLevelEnabled("verbose")) {
this.#log.verbose(` Required projects:`);
this.#log.verbose(` ${queue
.map((projectBuildContext) => {
const projectName = projectBuildContext.getProject().getName();
let msg;
Expand All @@ -225,7 +226,7 @@ class ProjectBuilder {
}

if (cleanDest) {
log.info(`Cleaning target directory...`);
this.#log.info(`Cleaning target directory...`);
await rimraf(destPath);
}
const startTime = process.hrtime();
Expand All @@ -234,29 +235,29 @@ class ProjectBuilder {
for (const projectBuildContext of queue) {
const projectName = projectBuildContext.getProject().getName();
const projectType = projectBuildContext.getProject().getType();
log.verbose(`Processing project ${projectName}...`);
this.#log.verbose(`Processing project ${projectName}...`);

// Only build projects that are not already build (i.e. provide a matching build manifest)
if (alreadyBuilt.includes(projectName)) {
log.skipProjectBuild(projectName, projectType);
this.#log.skipProjectBuild(projectName, projectType);
} else {
log.startProjectBuild(projectName, projectType);
this.#log.startProjectBuild(projectName, projectType);
await projectBuildContext.getTaskRunner().runTasks();
log.endProjectBuild(projectName, projectType);
this.#log.endProjectBuild(projectName, projectType);
}
if (!requestedProjects.includes(projectName)) {
// Project has not been requested
// => Its resources shall not be part of the build result
continue;
}

log.verbose(`Writing out files...`);
this.#log.verbose(`Writing out files...`);
pWrites.push(this._writeResults(projectBuildContext, fsTarget));
}
await Promise.all(pWrites);
log.info(`Build succeeded in ${this._getElapsedTime(startTime)}`);
this.#log.info(`Build succeeded in ${this._getElapsedTime(startTime)}`);
} catch (err) {
log.error(`Build failed in ${this._getElapsedTime(startTime)}`);
this.#log.error(`Build failed in ${this._getElapsedTime(startTime)}`);
throw err;
} finally {
this._deregisterCleanupSigHooks(cleanupSigHooks);
Expand All @@ -272,7 +273,7 @@ class ProjectBuilder {
const projectBuildContexts = new Map();

for (const projectName of requiredProjects) {
log.verbose(`Creating build context for project ${projectName}...`);
this.#log.verbose(`Creating build context for project ${projectName}...`);
const projectBuildContext = this._buildContext.createProjectContext({
project: this._graph.getProject(projectName)
});
Expand Down Expand Up @@ -320,15 +321,15 @@ class ProjectBuilder {

if (includedDependencies.length) {
if (includedDependencies.length === this._graph.getSize() - 1) {
log.info(` Including all dependencies`);
this.#log.info(` Including all dependencies`);
} else {
log.info(` Requested dependencies:`);
log.info(` + ${includedDependencies.join("\n + ")}`);
this.#log.info(` Requested dependencies:`);
this.#log.info(` + ${includedDependencies.join("\n + ")}`);
}
}
if (excludedDependencies.length) {
log.info(` Excluded dependencies:`);
log.info(` - ${excludedDependencies.join("\n + ")}`);
this.#log.info(` Excluded dependencies:`);
this.#log.info(` - ${excludedDependencies.join("\n + ")}`);
}

const rootProjectName = this._graph.getRoot().getName();
Expand Down Expand Up @@ -379,7 +380,7 @@ class ProjectBuilder {

await Promise.all(resources.map((resource) => {
if (taskUtil.getTag(resource, taskUtil.STANDARD_TAGS.OmitFromBuildResult)) {
log.silly(`Skipping write of resource tagged as "OmitFromBuildResult": ` +
this.#log.silly(`Skipping write of resource tagged as "OmitFromBuildResult": ` +
resource.getPath());
return; // Skip target write for this resource
}
Expand All @@ -388,7 +389,7 @@ class ProjectBuilder {
}

async _executeCleanupTasks() {
log.info("Executing cleanup tasks...");
this.#log.info("Executing cleanup tasks...");
await this._buildContext.executeCleanupTasks();
}

Expand Down
18 changes: 18 additions & 0 deletions test/lib/build/ProjectBuilder.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -731,6 +731,24 @@ test("_executeCleanupTasks", async (t) => {
"BuildContext#executeCleanupTasks got called with no arguments");
});

test("instantiate new logger for every ProjectBuilder", async (t) => {
function CreateBuildLoggerMock(moduleName) {
t.is(moduleName, "ProjectBuilder", "BuildLogger created with expected moduleName");
return {};
}

const {graph, taskRepository, sinon} = t.context;
const createBuildLoggerMockSpy = sinon.spy(CreateBuildLoggerMock);
const ProjectBuilder = await esmock("../../../lib/build/ProjectBuilder.js", {
"@ui5/logger/internal/loggers/Build": createBuildLoggerMockSpy
});

new ProjectBuilder({graph, taskRepository});
new ProjectBuilder({graph, taskRepository});

t.is(createBuildLoggerMockSpy.callCount, 2, "BuildLogger is instantiated for every ProjectBuilder instance");
});


function getProcessListenerCount() {
return ["SIGHUP", "SIGINT", "SIGTERM", "SIGBREAK"].map((eventName) => {
Expand Down

0 comments on commit f652461

Please sign in to comment.