Added Support for requirements.txt in venv create#63
Added Support for requirements.txt in venv create#63shreyasmene06 merged 16 commits intoshreyasmene06:mainfrom
Conversation
|
@shreyasmene06 kindly verify the pr |
There was a problem hiding this comment.
Nice clean implementation that addresses #56. The code is well-structured and the test coverage is good. Just one issue to fix:
Required Change
Remove requirements.txt from the PR
The file requirements.txt at the root with test data shouldn't be committed:
requests==2.31.0
click>=8.0.0
This appears to be a test file you created locally. The project already manages dependencies via pyproject.toml. Please remove this file from the commit.
git rm requirements.txt
git commit --amendMinor Suggestions (non-blocking)
-
Consider showing pip output on failure
Currently using capture_output=True which hides pip's output. For debugging, it might help users to see what went wrong. You could add a --verbose flag or print stderr on failure.
-
Upgrade pip first?
Some older venvs have outdated pip. Consider running pip install --upgrade pip before installing requirements, or document this as a known limitation.
Also make sure that the workflow passes all the checks.
Once the requirements.txt file is removed, this is good to merge!
|
@shreyasmene06 i have made the changes kindly verify the PR now |
@shreyasmene06 i have removed the requirements.txt file kindly merge the PR |
There was a problem hiding this comment.
Thanks for addressing the previous feedback! The test requirements.txt file was removed and error reporting was improved.
However, there's a bug in the pip command construction:
Required Fix
Duplicate pip arguments in venv.py (lines ~188-197)
pip_cmd.extend(["install", "-r", str(requirements_file)]) # <-- adds args here
# ...then later...
subprocess.run(
pip_cmd + ["install", "-r", str(requirements_file)], # <-- adds them AGAIN
...
)
This results in running:
pip install -r requirements.txt install -r requirements.txt
Fix: Remove one of the two. Either:
Option A - Remove the .extend():
# pip_cmd.extend(["install", "-r", str(requirements_file)]) # DELETE THIS LINE
subprocess.run(
pip_cmd + ["install", "-r", str(requirements_file)],
...
)
Option B - Use pip_cmd directly:
pip_cmd.extend(["install", "-r", str(requirements_file)])
subprocess.run(
pip_cmd, # Use directly, don't add more args
)Along with these please look for the code checks and the merge conflicts.
Once fixed, this is good to merge!
|
@shreyasmene06 i have made the changes and ran all the scripts you mentioned now kindly merge the PR |
shreyasmene06
left a comment
There was a problem hiding this comment.
The pip_cmd duplicate bug is fixed, but a new critical issue was introduced - likely from a bad merge:
Critical Bug
update_python_windows in installers.py is completely broken
The function now does validation and arch detection, but then just... ends. No return statement, no actual installation:
def update_python_windows(version_str: str, preferred: str = \"auto\") -> bool:
# ... validation and arch detection code ...
else:
arch = \"win32\"
# <-- Function ends here with no return or installation logic!
def update_python_linux(...): # Next function startsThis will cause update_python_windows to return None instead of bool, breaking Windows installation entirely.
This looks like a merge conflict was resolved incorrectly. The original code was:
def update_python_windows(version_str: str, preferred: str = "auto") -> bool:
return _install_with_plugins(version_str, preferred=preferred)
Please revert installers.py to keep just the original one-liner, or fix the merge properly.
Also
- Changes to tui.py seem unrelated to this PR (line wrapping change) - was this intentional?
Please fix the installers.py issue and I'm happy to approve!
also the checks have failed again please look into them
|
@shreyasmene06 kindly verify the PR now |
|
@shreyasmene06 pls verify the PR |
|
@shreyasmene06 kindly verify the PR |
shreyasmene06
left a comment
There was a problem hiding this comment.
All previous issues have been fixed:
update_python_windowsnow properly returns (was broken from bad merge)- Duplicate pip_cmd arguments fixed
- Test requirements.txt file removed
- Tests included
The implementation looks clean now: --requirements/-rflag properly integrated- Cross-platform pip handling (Windows Scripts/ vs Unix bin/)
- Fallback to
python -m pipwhen pip exe not found - Proper error handling with detailed output
- Test coverage included
Minor notes (non-blocking)
.flake8file: This is unrelated to the feature but harmless - consider adding in a separate PR for cleaner history.tui.pychange: This removes "W: wizard" from the hint bar. Since PR #70 (wizard) was already merged, this might cause a merge conflict or revert that addition. Please check when merging.
LGTM!
fixes #56
i updated cli.py so the create command now accepts a --requirements flag. then i modified venv.py to actually run pip install inside the new venv if you provide that file. i also added a test case to test_venv.py to make sure it works properly.