fix-windows-python-init#10608
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the Python virtual environment setup and dependency installation during functions initialization by capturing process exit codes and throwing user-friendly FirebaseErrors on failure. It also adds corresponding unit tests. The review feedback correctly points out that if the spawned processes fail to execute (e.g., due to a missing binary), the 'error' event will reject the promise and bypass the FirebaseError checks. Resolving the promises with a non-zero exit code on error will ensure these failures are handled gracefully.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the Python functions initialization process by introducing helper functions for virtual environment creation and dependency installation, resolving version-specific Python binaries, and upgrading pip safely on Windows. It also adds comprehensive unit tests for these flows. The reviewer suggests extracting the duplicated promise-based process waiting logic across the new helper functions into a single reusable waitForProcess helper to reduce duplication and align with the repository's style guide.
ajperel
left a comment
ajperel
left a comment
There was a problem hiding this comment.
I'm very unfamiliar with this code. It seems fine but might not be a bad idea to get a second review from someone more familiar. Left one quick comment.
Also... a few warnings in the tests that would be good to resolve.
| * Helper to spawn a process and create a python virtual environment. | ||
| */ | ||
| async function createVirtualEnv(pythonBinary: string, cwd: string): Promise<void> { | ||
| const venvProcess = spawn(pythonBinary, ["-m", "venv", "venv"], { |
There was a problem hiding this comment.
I think you can be consistent with other code and simplify this abit by using our wrapspawn utility:
firebase-tools/src/init/spawn.ts
Line 11 in 938d93c
3 participants
This pull request fixes a bug on Windows where initializing Firebase Functions with Python fails during the dependency installation step, throwing a misleading error that says it cannot find the file activate.bat (ENOENT).
The Problem
During investigation, we found that this error is caused by two different issues. First, if a user does not have Python installed or configured in their system PATH, the step to create the virtual environment fails silently because the Firebase CLI does not check its exit code. The CLI then moves on to execute the activation script, which fails because the virtual environment folder was never actually created.
Second, even if a user has Python installed, the installation command tries to run pip3 directly to upgrade pip. On Windows, the operating system locks the pip3 executable while it is running, which causes the upgrade to fail. In both of these failure cases, a quirk in the command-spawning library on Windows translates the failure into a misleading "file not found" error for the activate.bat file, hiding the real root cause.
The Solution
To fix these issues, we updated the virtual environment creation step to output python errors directly to the terminal and to verify the command's exit code. If the virtual environment fails to build (due to a missing Python installation), the CLI now stops immediately and shows a clear error message.
We also changed the dependency commands to use python -m pip instead of calling pip3 directly. This approach is the recommended standard because it runs the command through the Python interpreter, avoiding the Windows file-locking issue. Finally, we added unit tests to ensure that the Python codebase is initialized with the correct configuration and that failures are caught and reported properly.