gnodet commented on code in PR #24665: URL: https://github.com/apache/camel/pull/24665#discussion_r3572911845
########## tooling/camel-exe/src/main/native/camel.c: ########## @@ -0,0 +1,106 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +/* + * camel.exe - minimal WinGet-compatible bootstrap for the Camel CLI launcher. + * + * It locates the camel.bat sitting in the same directory, forwards the caller's + * exact command-line tail (preserving Unicode and Windows quoting), inherits the + * standard streams, and returns the child process exit code. It performs NO Java + * discovery and NO Camel option parsing; that all lives in camel.bat. + */ + +#include <windows.h> +#include <stdio.h> +#include <stdlib.h> +#include <wchar.h> + +/* Return the command-line tail after argv[0], preserving the caller's quoting. */ +static LPWSTR skip_argv0(LPWSTR cmd) { + LPWSTR p = cmd; + if (*p == L'"') { + p++; + while (*p && *p != L'"') { + p++; + } + if (*p == L'"') { + p++; + } + } else { + while (*p && *p != L' ' && *p != L'\t') { + p++; + } + } + while (*p == L' ' || *p == L'\t') { + p++; + } + return p; +} + +int wmain(void) { + wchar_t exePath[MAX_PATH]; + DWORD n = GetModuleFileNameW(NULL, exePath, MAX_PATH); + if (n == 0 || n >= MAX_PATH) { + fwprintf(stderr, L"camel: launcher path exceeds MAX_PATH (%d chars) or cannot be resolved\n", MAX_PATH); + return 1; + } + + /* Strip the exe filename, leaving the directory. */ + wchar_t *slash = wcsrchr(exePath, L'\\'); + if (slash == NULL) { + fwprintf(stderr, L"camel: unexpected launcher path\n"); + return 1; + } + *slash = L'\0'; + + LPWSTR tail = skip_argv0(GetCommandLineW()); + + /* + * cmd.exe /S /C ""<dir>\camel.bat" <tail>" + * With /S and a command wrapped in an outer pair of quotes, cmd strips the + * first and last quote and executes the remainder verbatim, so the inner + * quotes around the (possibly spaced/Unicode) camel.bat path survive. + */ + size_t cap = wcslen(exePath) + wcslen(tail) + 64; + LPWSTR cmdline = (LPWSTR) malloc(cap * sizeof(wchar_t)); + if (cmdline == NULL) { + fwprintf(stderr, L"camel: out of memory\n"); + return 1; + } + _snwprintf_s(cmdline, cap, _TRUNCATE, + L"cmd.exe /S /C \"\"%s\\camel.bat\" %s\"", exePath, tail); + Review Comment: [MEDIUM] **Silent truncation risk**: `_snwprintf_s` with `_TRUNCATE` will silently truncate the formatted string if it exceeds `cap`. While the `+64` margin (the fixed overhead is ~31 chars) makes this practically impossible for realistic paths, checking the return value would guard against silent malformation. Consider checking the return value: ```c int written = _snwprintf_s(cmdline, cap, _TRUNCATE, L"cmd.exe /S /C \"\"%s\\camel.bat\" %s\"", exePath, tail); if (written < 0) { fwprintf(stderr, L"camel: command line too long\n"); free(cmdline); return 1; } ``` _This review was generated by an AI agent and may contain inaccuracies._ ########## tooling/camel-exe/src/main/native/camel.c: ########## @@ -0,0 +1,106 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +/* + * camel.exe - minimal WinGet-compatible bootstrap for the Camel CLI launcher. + * + * It locates the camel.bat sitting in the same directory, forwards the caller's + * exact command-line tail (preserving Unicode and Windows quoting), inherits the + * standard streams, and returns the child process exit code. It performs NO Java + * discovery and NO Camel option parsing; that all lives in camel.bat. + */ + +#include <windows.h> +#include <stdio.h> +#include <stdlib.h> +#include <wchar.h> + +/* Return the command-line tail after argv[0], preserving the caller's quoting. */ +static LPWSTR skip_argv0(LPWSTR cmd) { + LPWSTR p = cmd; + if (*p == L'"') { + p++; + while (*p && *p != L'"') { + p++; + } + if (*p == L'"') { + p++; + } + } else { + while (*p && *p != L' ' && *p != L'\t') { + p++; + } + } + while (*p == L' ' || *p == L'\t') { + p++; + } + return p; +} + +int wmain(void) { + wchar_t exePath[MAX_PATH]; + DWORD n = GetModuleFileNameW(NULL, exePath, MAX_PATH); Review Comment: [LOW] **MAX_PATH limitation**: On Windows 10+ with long path support enabled (`LongPathsEnabled` registry key), paths can exceed 260 characters. A dynamic allocation loop (doubling the buffer until `GetModuleFileNameW` succeeds with `GetLastError() != ERROR_INSUFFICIENT_BUFFER`) would future-proof this, but it is not a blocker for the initial contribution. _This review was generated by an AI agent and may contain inaccuracies._ ########## .github/workflows/camel-launcher-windows.yml: ########## @@ -0,0 +1,127 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +name: Camel launcher Windows build + +on: + push: + branches: + - main + pull_request: + branches: + - main + workflow_dispatch: + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +permissions: + contents: read + +jobs: + detect-changes: + runs-on: ubuntu-latest + outputs: + camel-exe: ${{ steps.changes.outputs.camel-exe }} + camel-launcher: ${{ steps.changes.outputs.camel-launcher }} + steps: + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + persist-credentials: false + fetch-depth: 0 + - id: changes + run: | + if [ "${{ github.event_name }}" = "workflow_dispatch" ]; then + echo "camel-exe=true" >> "$GITHUB_OUTPUT" + echo "camel-launcher=true" >> "$GITHUB_OUTPUT" + exit 0 + fi + + if [ "${{ github.event_name }}" = "pull_request" ]; then + git fetch origin "${{ github.base_ref }}" + CHANGED="$(git diff --name-only "origin/${{ github.base_ref }}"...HEAD)" + else + CHANGED="$(git diff --name-only "${{ github.event.before }}" "${{ github.sha }}")" + fi + + echo "Changed files:" + echo "$CHANGED" + + camel_exe=false + camel_launcher=false + + if echo "$CHANGED" | grep -qE '^(tooling/camel-exe/|\.github/workflows/camel-launcher-windows\.yml)'; then + camel_exe=true + fi + + if echo "$CHANGED" | grep -qE '^(dsl/camel-jbang/camel-launcher/|tooling/camel-exe/|\.github/workflows/camel-launcher-windows\.yml)'; then + camel_launcher=true + fi + + echo "camel-exe=$camel_exe" >> "$GITHUB_OUTPUT" + echo "camel-launcher=$camel_launcher" >> "$GITHUB_OUTPUT" + + camel-exe: + needs: detect-changes + if: github.event_name == 'workflow_dispatch' || needs.detect-changes.outputs.camel-exe == 'true' + runs-on: windows-2022 + steps: + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + persist-credentials: false + - name: Set up JDK 21 + uses: actions/setup-java@be666c2fcd27ec809703dec50e508c2fdc7f6654 # v5.2.0 + with: + distribution: 'temurin' + java-version: '21' + cache: 'maven' + - name: Build and test native camel.exe + shell: cmd + run: | + for /f "usebackq delims=" %%i in (`"%ProgramFiles(x86)%\Microsoft Visual Studio\Installer\vswhere.exe" -latest -products * -requires Microsoft.VisualStudio.Component.VC.Tools.x86.x64 -property installationPath`) do set "VSINSTALL=%%i" + call "%VSINSTALL%\VC\Auxiliary\Build\vcvars64.bat" + mvn -B -ntp -pl buildingtools install -DskipTests + mvn -B -ntp -pl tooling/camel-exe verify -Dcamel.exe.requireWindowsExe=true + + camel-launcher-windows: + needs: detect-changes + if: github.event_name == 'workflow_dispatch' || needs.detect-changes.outputs.camel-launcher == 'true' + runs-on: windows-2022 + steps: + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + persist-credentials: false + - name: Set up JDK 21 + uses: actions/setup-java@be666c2fcd27ec809703dec50e508c2fdc7f6654 # v5.2.0 + with: + distribution: 'temurin' + java-version: '21' + cache: 'maven' + - name: Build launcher with native camel.exe and run tests + shell: cmd + run: | + for /f "usebackq delims=" %%i in (`"%ProgramFiles(x86)%\Microsoft Visual Studio\Installer\vswhere.exe" -latest -products * -requires Microsoft.VisualStudio.Component.VC.Tools.x86.x64 -property installationPath`) do set "VSINSTALL=%%i" + call "%VSINSTALL%\VC\Auxiliary\Build\vcvars64.bat" + mvn -B -ntp -pl buildingtools,tooling/camel-exe,dsl/camel-jbang/camel-launcher -am install -Dcamel.launcher.requireWindowsExe=true + - name: Assert archive carries bin/camel.exe + shell: pwsh + run: | + $zip = Get-ChildItem dsl/camel-jbang/camel-launcher/target/camel-launcher-*-bin.zip | Select-Object -First 1 + Add-Type -AssemblyName System.IO.Compression.FileSystem + $names = [System.IO.Compression.ZipFile]::OpenRead($zip.FullName).Entries.FullName + if ($names -notcontains 'bin/camel.exe') { throw 'camel.exe missing from launcher archive' } Review Comment: [INFO] **CI failure note**: The `camel-launcher-windows` job uses `-am` which pulls in the full upstream dependency tree. The current CI failure is in `camel-management` (unrelated to this PR). Consider whether the Maven command here could add `-DskipTests` and rely on the dedicated `camel-exe` job for test execution, plus the PowerShell archive assertion below for distribution validation. This would isolate the launcher build from unrelated test failures in upstream modules. _This review was generated by an AI agent and may contain inaccuracies._ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
