Skip to content

Use shell=False for Popen on Windows #1324

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

michaelosthege
Copy link
Member

@michaelosthege michaelosthege commented Mar 27, 2025

Description

I'm having hard times creating new environments with working compiler dependencies.

My debugging attempts show that shell=False could fix this, however this observation contradicts the comments next to that line.
Further testing may be required. Tested on another machine.

I also removed an old workaround for Python 2..

Related Issue

Hints?

Checklist

Type of change

  • New feature / enhancement
  • Bug fix
  • Documentation
  • Maintenance
  • Other (please specify):

📚 Documentation preview 📚: https://pytensor--1324.org.readthedocs.build/en/1324/

@michaelosthege michaelosthege added bug Something isn't working C-backend labels Mar 27, 2025
Copy link

codecov bot commented Mar 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.05%. Comparing base (f1514eb) to head (c78cef1).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1324   +/-   ##
=======================================
  Coverage   82.05%   82.05%           
=======================================
  Files         203      203           
  Lines       48863    48863           
  Branches     8695     8695           
=======================================
  Hits        40093    40093           
  Misses       6619     6619           
  Partials     2151     2151           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@michaelosthege
Copy link
Member Author

@Armavica you also worked on these components - what do you think about shell=False? Does it work on your machine?

@Armavica
Copy link
Member

I will have a look this weekend or next week

@michaelosthege
Copy link
Member Author

I will have a look this weekend or next week

@Armavica did you already get a chance?

@Armavica
Copy link
Member

Armavica commented Apr 10, 2025

I don't think I worked specifically on this besides cosmetic changes, and I never tried to install PyTensor on Windows.
However, I think if we can make things work with shell=False it's probably for the best. Have you tried without the list to string conversion that follows, that is supposed to be only needed for shell=True?

michaelosthege and others added 2 commits April 12, 2025 15:48
The backslashes in `__file__` created invalid escape sequences.
@michaelosthege michaelosthege marked this pull request as ready for review April 12, 2025 14:00
@michaelosthege
Copy link
Member Author

Have you tried without the list to string conversion that follows

Without the list to string conversion already the first test failed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working C-backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants