Skip to content
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

sage_setup: Proper error checking for makedirs #23774

Closed
mkoeppe opened this issue Sep 1, 2017 · 19 comments
Closed

sage_setup: Proper error checking for makedirs #23774

mkoeppe opened this issue Sep 1, 2017 · 19 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 1, 2017

If the Python function makedirs is used to achieve the effect of mkdir -p, one needs to distinguish two situations: Directory already exists; directory does not exist and cannot be created.

Current code in sage_setup does not raise an error in the latter situation. Noticed while working on #21469 (VPATH).

See https://stackoverflow.com/questions/600268/mkdir-p-functionality-in-python

CC: @jhpalmieri @jdemeyer @embray

Component: build

Author: Matthias Koeppe

Branch/Commit: 01c3b38

Reviewer: Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/23774

@mkoeppe mkoeppe added this to the sage-8.1 milestone Sep 1, 2017
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 1, 2017

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 1, 2017

Author: Matthias Koeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 1, 2017

Commit: 86f742b

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 1, 2017

New commits:

86f742bsage_setup: Better error checking for makedirs

@jdemeyer
Copy link

jdemeyer commented Sep 2, 2017

comment:3

I prefer a simpler

if not os.path.isdir(path):
    raise

in the except block. Why do you need to check in addition the errno?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Sep 2, 2017

comment:4

I'd say not checking the errno is the moral equivalent of catching all exceptions.

@fchapoton
Copy link
Contributor

comment:5

does not build ("path" is not defined..)

@jdemeyer
Copy link

jdemeyer commented Sep 2, 2017

comment:6

Replying to @mkoeppe:

I'd say not checking the errno is the moral equivalent of catching all exceptions.

It's fine to catch all exceptions because I suggest to check the desired result not the reason why it failed.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 2, 2017

Changed commit from 86f742b to 1970398

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 2, 2017

Branch pushed to git repo; I updated commit sha1. New commits:

1970398Fix and simplify last change

@jdemeyer
Copy link

jdemeyer commented Sep 3, 2017

comment:8

Obvious simplification: change

if condition:
    pass
else:
    raise

to

if not condition:
    raise

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 3, 2017

Changed commit from 1970398 to 7e05ec6

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 3, 2017

Branch pushed to git repo; I updated commit sha1. New commits:

7e05ec6Further jeroenification

@jdemeyer
Copy link

jdemeyer commented Sep 4, 2017

@jdemeyer
Copy link

jdemeyer commented Sep 4, 2017

comment:11

I don't like your last commit message, so I squashed these commits to one.


New commits:

01c3b38sage_setup: Better error checking for makedirs

@jdemeyer
Copy link

jdemeyer commented Sep 4, 2017

Changed commit from 7e05ec6 to 01c3b38

@jdemeyer
Copy link

jdemeyer commented Sep 4, 2017

Reviewer: Jeroen Demeyer

@vbraun
Copy link
Member

vbraun commented Sep 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants