Skip to content

Conversation

@kraenhansen
Copy link
Collaborator

Merging this PR will:

  • Throw an error when no explicit targets are provided and no default targets are found.

@kraenhansen kraenhansen self-assigned this Jun 19, 2025
@changeset-bot
Copy link

changeset-bot bot commented Jun 19, 2025

⚠️ No Changeset found

Latest commit: ffd08dc

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@kraenhansen kraenhansen added the CMake RN Our `cmake` wrapping CLI label Jun 19, 2025
@kraenhansen kraenhansen requested a review from Copilot June 19, 2025 13:58
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a guard to error out when no default build triplets are found, instead of proceeding silently.

  • Checks if triplets is empty and throws a user-facing error.
  • Moves the default-triplet info log into an else branch.
Comments suppressed due to low confidence (1)

packages/cmake-rn/src/cli.ts:148

  • [nitpick] Hyphenate 'platform-specific' and consider adding guidance on how to explicitly provide targets (e.g., --targets) to help users resolve this error.
            "Found no default triplets: Install some platform specific build tools"

Comment on lines +147 to +149
throw new Error(
"Found no default triplets: Install some platform specific build tools"
);
Copy link

Copilot AI Jun 19, 2025

Choose a reason for hiding this comment

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

Consider using commander’s program.error() or printing a clear message with console.error() followed by process.exit(1) instead of throwing a raw Error, to avoid exposing a stack trace in normal CLI usage.

Suggested change
throw new Error(
"Found no default triplets: Install some platform specific build tools"
);
console.error(
chalk.redBright("✖"),
"Found no default triplets: Install some platform specific build tools"
);
process.exit(1);

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.

I'd like to postpone this to another PR copying the error handling from ferric somewhere shared between the different CLIs:

if (error instanceof UsageError || error instanceof SpawnFailure) {
console.error(chalk.red("ERROR"), error.message);
if (error.cause instanceof Error) {
console.error(chalk.red("CAUSE"), error.cause.message);
}
if (error instanceof UsageError && error.fix) {
console.error(
chalk.green("FIX"),
error.fix.command
? chalk.dim("Run: ") + error.fix.command
: error.fix.instructions
);
}
} else {

@kraenhansen kraenhansen merged commit 45e472b into main Jun 20, 2025
4 checks passed
@kraenhansen kraenhansen deleted the kh/no-default-targets-error branch June 20, 2025 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CMake RN Our `cmake` wrapping CLI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants