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

limit filelock use #2221

Merged
merged 1 commit into from
Oct 28, 2016
Merged

limit filelock use #2221

merged 1 commit into from
Oct 28, 2016

Conversation

marqh
Copy link
Member

@marqh marqh commented Oct 27, 2016

remove the behaviour, introduced in #2192, where installations without write permissions to the iris source directories are unable to run tests, due to

IOError: [Errno 13] Permission denied: 'imagerepo.lock'

@bjlittle does this maintain your desired behaviour for developers whilst removing the installation and testing block?

with open(repo_fname, 'wb') as fo:
json.dump(repo, codecs.getwriter('utf-8')(fo), indent=4,
sort_keys=True)
lock = filelock.FileLock(repo_fname)
Copy link
Member

Choose a reason for hiding this comment

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

This looks like an improvement over the initial proposal.

@lbdreyer
Copy link
Member

Thanks @marqh :)

@lbdreyer lbdreyer merged commit e3bc58d into SciTools:v1.11.x Oct 28, 2016
@bjlittle
Copy link
Member

bjlittle commented Oct 28, 2016

@marqh @lbdreyer @dkillick You've introduced a bug here ...

The lock file imagerepo.lock is a separate file from the file imagerepo.json that the lock protects access to in the lock critical region.

The code change that @lbdreyer merged uses the imagerepo.json file as the actual lock file in error. That's bad.

Line 737 should be replaced with the following:

lock = filelock.FileLock(os.path.join(_RESULT_PATH, 'imagerepo.lock'))

And line 811 now requires to be deleted ... infact the check_graphic function no longer needs to be a wrapper function to _assert_graphic. It should be the case now that _assert_graphic is check_graphic.

Someone needs to fix this now before v1.11.x is cut

@bjlittle bjlittle added this to the v1.11 milestone Oct 28, 2016
@lbdreyer
Copy link
Member

Interesting that the test still pass 😕

Thanks for catching this @bjlittle!

I have now put up PR #2225 that should hopefully fix it.

@lbdreyer lbdreyer mentioned this pull request Oct 28, 2016
@bjlittle
Copy link
Member

@lbdreyer There are no tests to test the testing framework.

@DPeterK
Copy link
Member

DPeterK commented Oct 31, 2016

There are no tests to test the testing framework

This has always struck me as an abrupt irony.

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.

5 participants