Skip to content

Fix expansion of options in --build and --out #278

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Oct 21, 2025
Merged

Fix expansion of options in --build and --out #278

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
753c553
Fix expansion of options in --build and --out
kraenhansen Oct 20, 2025
cf53c58
Attempt at correcting paths on windows
kraenhansen Oct 21, 2025
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
5 changes: 5 additions & 0 deletions .changeset/fresh-frogs-enter.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"cmake-rn": patch
---

Fix expansion of options in --build and --out
35 changes: 22 additions & 13 deletions packages/cmake-rn/src/cli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ const tripletOption = new Option(
const buildPathOption = new Option(
"--build <path>",
"Specify the build directory to store the configured CMake project",
);
).default("{source}/build");

const cleanOption = new Option(
"--clean",
Expand All @@ -84,7 +84,7 @@ const cleanOption = new Option(
const outPathOption = new Option(
"--out <path>",
"Specify the output directory to store the final build artifacts",
).default(false, "./{build}/{configuration}");
).default("{build}/{configuration}");

const defineOption = new Option(
"-D,--define <entry...>",
Expand Down Expand Up @@ -151,8 +151,27 @@ for (const platform of platforms) {
program = platform.amendCommand(program);
}

function expandTemplate(
input: string,
values: Record<string, unknown>,
): string {
Comment on lines +154 to +157
Copy link

Copilot AI Oct 21, 2025

Choose a reason for hiding this comment

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

The expandTemplate function lacks documentation explaining the template syntax, expected placeholders (e.g., {source}, {build}, {configuration}), and behavior when values are missing or non-string. Add a JSDoc comment describing the template format and edge case handling.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🤟

return input.replaceAll(/{([^}]+)}/g, (_, key: string) =>
typeof values[key] === "string" ? values[key] : "",
Copy link

Copilot AI Oct 21, 2025

Choose a reason for hiding this comment

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

The type guard typeof values[key] === 'string' only handles string values, but the values parameter is typed as Record<string, unknown>. Non-string values like numbers or booleans will be silently replaced with empty strings. Consider converting non-string values to strings using String(values[key]) or documenting why only strings should be expanded.

Suggested change
typeof values[key] === "string" ? values[key] : "",
String(values[key]),

Copilot uses AI. Check for mistakes.
);
Comment on lines +158 to +160
Copy link

Copilot AI Oct 21, 2025

Choose a reason for hiding this comment

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

The template expansion returns an empty string for non-string values, which could silently produce incorrect paths. For example, if {configuration} is undefined or non-string, the resulting path would be malformed (e.g., build//artifacts). Consider throwing an error for missing or non-string values, or at minimum documenting this behavior.

Suggested change
return input.replaceAll(/{([^}]+)}/g, (_, key: string) =>
typeof values[key] === "string" ? values[key] : "",
);
return input.replaceAll(/{([^}]+)}/g, (_, key: string) => {
if (typeof values[key] !== "string") {
throw new Error(`Template variable '{${key}}' is missing or not a string (got: ${typeof values[key]})`);
}
return values[key] as string;
});

Copilot uses AI. Check for mistakes.
}

program = program.action(
wrapAction(async ({ triplet: requestedTriplets, ...baseOptions }) => {
baseOptions.build = path.resolve(
process.cwd(),
expandTemplate(baseOptions.build, baseOptions),
);
baseOptions.out = path.resolve(
process.cwd(),
expandTemplate(baseOptions.out, baseOptions),
Copy link

Copilot AI Oct 21, 2025

Choose a reason for hiding this comment

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

The out option expansion on line 171 references {build} in its default value ({build}/{configuration}), but the template is expanded using the original baseOptions.build string (e.g., {source}/build) rather than the already-resolved build path. This creates a circular dependency issue where out will contain the literal string {source}/build/{configuration} instead of the resolved path. The expansion of out should use the already-resolved build value.

Suggested change
expandTemplate(baseOptions.out, baseOptions),
expandTemplate(
baseOptions.out,
{ ...baseOptions, build: baseOptions.build }
),

Copilot uses AI. Check for mistakes.
);
const { out, build: buildPath } = baseOptions;

assertFixable(
fs.existsSync(path.join(baseOptions.source, "CMakeLists.txt")),
`No CMakeLists.txt found in source directory: ${chalk.dim(baseOptions.source)}`,
Expand All @@ -161,7 +180,6 @@ program = program.action(
},
);

const buildPath = getBuildPath(baseOptions);
if (baseOptions.clean) {
await fs.promises.rm(buildPath, { recursive: true, force: true });
}
Expand Down Expand Up @@ -197,10 +215,6 @@ program = program.action(
}
}

if (!baseOptions.out) {
baseOptions.out = path.join(buildPath, baseOptions.configuration);
}

const tripletContexts = [...triplets].map((triplet) => {
const platform = findPlatformForTriplet(triplet);
const tripletBuildPath = getTripletBuildPath(buildPath, triplet);
Expand Down Expand Up @@ -262,7 +276,7 @@ program = program.action(
}
await platform.postBuild(
{
outputPath: baseOptions.out || baseOptions.source,
outputPath: out,
triplets: relevantTriplets,
},
baseOptions,
Expand All @@ -288,11 +302,6 @@ function getTripletsSummary(
.join(" / ");
}

function getBuildPath({ build, source }: BaseOpts) {
// TODO: Add configuration (debug vs release)
return path.resolve(process.cwd(), build || path.join(source, "build"));
}

/**
* Namespaces the output path with a triplet name
*/
Expand Down
Loading