Skip to content

Add --out-to-build option #148

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.

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

Closed
wants to merge 1 commit into from
Closed

Conversation

kraenhansen
Copy link
Collaborator

@kraenhansen kraenhansen commented Jun 29, 2025

As a quick fix to address the issue of the output directory being misaligned with cmake-js and node-gyp (mentioned in #146).

Merging this PR will:

  • Introduce a --out-to-build option which will output the prebuild artifact in the build directory (under "build/Release" or "build/Debug"), just like cmake-js and node-gyp does.

We might want to make this the default behaviour at some point.

@kraenhansen kraenhansen self-assigned this Jun 29, 2025
@kraenhansen kraenhansen added enhancement New feature or request CMake RN Our `cmake` wrapping CLI labels Jun 29, 2025
@kraenhansen kraenhansen requested a review from Copilot July 2, 2025 10:50
Copy link

@Copilot 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 new --out-to-build CLI flag so that artifacts are placed under build/<configuration>, mirroring cmake-js and node-gyp, and renames the existing outPathOption variable while adding logic to ensure --out and --out-to-build are mutually exclusive.

  • Introduce --out-to-build option in cli.ts
  • Rename outPathOption variable to outOption
  • Add runtime assertion to prevent using --out with --out-to-build
Comments suppressed due to low confidence (2)

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

  • [nitpick] Renaming outPathOption to outOption reduces clarity and consistency. Consider reverting to outPathOption or using outDirOption to more clearly describe its purpose.
const outOption = new Option(

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

  • There are no tests verifying the new --out-to-build behavior or its conflict with --out. Adding unit tests for both the default and conflict scenarios will help prevent regressions.
      if (globalContext.outToBuild) {

@@ -169,6 +175,14 @@ export const program = new Command("cmake-rn")
}
}

if (globalContext.outToBuild) {
Copy link
Preview

Copilot AI Jul 2, 2025

Choose a reason for hiding this comment

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

Instead of manually asserting mutual exclusion at runtime, use Commander.js's .conflicts() API to declare that --out and --out-to-build cannot be used together, providing built-in CLI validation and clearer error messaging.

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.

We'll want to make this the default behavior and as such, I expect it to just end up as default for the --out parameter.

@kraenhansen kraenhansen force-pushed the main branch 3 times, most recently from cf4d3b7 to c18a1be Compare July 7, 2025 12:01
@kraenhansen
Copy link
Collaborator Author

Closing as superseded by #150

@kraenhansen kraenhansen closed this Jul 7, 2025
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 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant