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

Fix @cachable decorator for windows. #9

Closed
wants to merge 2 commits into from

Conversation

euronion
Copy link
Contributor

For windows paths, this returned e.g. "C" (for paths like "C:\Temp\vresutils") which is an invalid input for os.stat.

IMHO: How could this ever have worked?

For windows paths, this returned e.g. "C" (for paths like "C:\Temp\vresutils\") which is an invalid input for `os.stat`.

IMHO: How could this ever have worked?
Copy link
Member

@coroa coroa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to use cachable on windows, you have to take out or at least make all of the permission handling optional; ie.

https://github.com/FRESNA/vresutils/blob/master/vresutils/decorators.py#L70-L75
should be skipped on windows, as well as

if gid: os.fchown(f.fileno(), -1, gid)
if mode: os.fchmod(f.fileno(), mode)


if enable:
st = os.stat(cache_dir[0])
st = os.stat(root_dir(cache_dir))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
st = os.stat(root_dir(cache_dir))
st = os.stat(cache_dir)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code in the if enable clause was supposed to get the unix permissions of the cache directory, so that later on we can set the same permissions on the cache files.

I mistakenly introduced the [0] back when I added the fallback_cache_dirs (80c9997). In some intermediate state cache_dir was a list of cache directories and then you wanted the first one from the list.

@euronion
Copy link
Contributor Author

TBH I did not time looking into what this function does nor did I use it intentionally. My snakemake workflow in pypsa-eur just failed because of this during some work on my windows machine and I needed a quick fix.

If I understand you correctly, this code shouldn't have been called on my windows machine?

@coroa
Copy link
Member

coroa commented Nov 27, 2019

The default should have been to just shut up, if the cache_dir directory, which you configure in your .vresutils.config does not exist. Maybe just remove or comment that particular line from your config. Then it should ignore all cachable stuff.

If you don't have any .vresutils.config it should also just work.

@euronion
Copy link
Contributor Author

I was expecting it to work out-of-the-box as you describe it.

If you don't have any .vresutils.config it should also just work.

Mea culpa. Somehow somewhen I created this file causing the problem.
Deleting it resolves the problem.

Thanks a lot for the hint. From my side we can close w/o merge.

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

Successfully merging this pull request may close these issues.

2 participants