#27359 closed defect (fixed)

Test for sage_makedirs breaks Sage on Windows

Reported by: embray Owned by:
Priority: blocker Milestone: sage-8.7
Component: misc Keywords:
Cc: Merged in:
Authors: Erik Bray Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 87bc817 (Commits) Commit: 87bc8177ff587434d61c05a4889e89aecdc29f74
Dependencies: Stopgaps:

Description

#27196 modified a test for sage_makedirs to try to make a directory in place of the actual Python executable (sys.executable).

This test has a number of problems, not the least of which being that it will fail for permission reasons when run in a distro like Python.

But it actually breaks Sage on Windows because it succeeds in creating a directory called "python2" alongside the actual executable "python2.exe".

I think the test itself is a bit silly since it's just testing standard functionality of the stdlib's os.makedirs, but if we want to keep it for example purposes I would rewrite it to actually use a temp file and not try to overwrite a real file used by the system.

Change History (8)

comment:1 Changed 12 months ago by embray

  • Authors set to Erik Bray
  • Branch set to u/embray/ticket-27359
  • Commit set to a6bc173a66694b6c914932c7c5b15dacdf9309e1
  • Status changed from new to needs_review

Still confirming that the updated test works on Python 3, but I believe it will.


New commits:

a6bc173Trac #27359: Rewrite this test to use a temp file instead of risk

comment:2 follow-up: Changed 12 months ago by jdemeyer

tmp_filename() already creates the file. There is no need to open and close it a second time.

comment:3 in reply to: ↑ description Changed 12 months ago by jdemeyer

Replying to embray:

I think the test itself is a bit silly since it's just testing standard functionality of the stdlib's os.makedirs

Not really, as os.makedirs fails if the directory already exists. (In Python 3, this was improved by adding support for an exist_ok=True flag)

comment:4 Changed 12 months ago by embray

Confirmed this passes for me on Python 3.

comment:5 in reply to: ↑ 2 Changed 12 months ago by embray

Replying to jdemeyer:

tmp_filename() already creates the file. There is no need to open and close it a second time.

Ah, so it does. I'll just fix that real quick.

comment:6 Changed 12 months ago by git

  • Commit changed from a6bc173a66694b6c914932c7c5b15dacdf9309e1 to 87bc8177ff587434d61c05a4889e89aecdc29f74

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

a971864Trac #27359: Rewrite this test to use a temp file instead of risk
87bc817trivial: change argument name in this function to not shadow the `dir` builtin

comment:7 Changed 12 months ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:8 Changed 11 months ago by vbraun

  • Branch changed from u/embray/ticket-27359 to 87bc8177ff587434d61c05a4889e89aecdc29f74
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.