Add warning for netCDF4 bug

[braaannigan]

Closes #1745 by adding a warning message for a bug in netCDF4. The bug occurs when the filepath has 88 characters and the netCDF4 engine has been used with version less than 1.3.1.

The warning message has been added at the first point where the length of the file path is known and the netcdf4 version number is known.

I don't think there is any corresponding test, as the bug leads to a seg fault rather than a python output/exception.

Passes git diff upstream/master **/*py | flake8 --diff (remove if you did not edit any Python files)

Updated whats-new.rst

[shoyer]

Thanks! A good way to test this would be to use mock, e.g.,

with mock.patch('netCDF4.__version__', '1.2.4'):
    with pytest.warns(Warning, match='segmentation fault'):
        xarray.Dataset().to_netcdf('a' * 88)

[shoyer]

Use LooseVersion() here: https://stackoverflow.com/questions/11887762/compare-version-strings-in-python

[shoyer]

Please remove spaces around filename_or_obj, per pep8.

[shoyer]

Can you put this inside NetCDF4DataStore.open() instead? We try to keep the backend specific logic in the backend classes.

[braaannigan]

Thanks for the advice @shoyer

I've moved the warning to NetCDF4DataStore.open() and reverted backends/api.py back to the original.

I've used LooseVersion for the version comparison and made the pep8 change.

There is a problem with the test. I've added the test to NetCDF4DataTest. However, when the filename has 88 characters the seg fault occurs and the test gets stopped. The output from

$py.test tests/test_backends.py

is just:

============================= test session starts ==============================
platform linux -- Python 3.6.2, pytest-3.2.1, py-1.4.34, pluggy-0.4.0
rootdir: /my_python_path/xarray, inifile: pytest.ini
plugins: pylama-7.4.3
collected 1042 items

tests/test_backends.py .
*** Error in `/path_to/python': double free or corruption (!prev):  ***
Aborted (core dumped)

If the filename doesn't have 88 characters then the test result is "Failed: DID NOT WARN.", which is fine as it shouldn't trigger the warning.

I've been trying to see how people handle this issue, but haven't been able to find examples of people trying to trigger seg faults in a test yet

[braaannigan]

There are some other small issues I need to deal with, I'll leave a message when I've sorted those out

[shoyer]

@braaannigan How about we turn warnings into errors, and then catch the appropriate error instead? e.g.,

with warnings.catch_warnings():
    warnings.simplefilter("error")
    with raises_regex(Warning, 'segmentation fault'):
        ...

That should abort (due to the error) before the seg fault.

[shoyer]

You can put this import at the top level. (We only import netCDF4 inside functions/methods to avoid the hard dependency, but disutils is part of Python's standard library.)

[braaannigan]

I've pushed an updated version with a successful test on my machine.

I've modified the dataset name in the test case to ensure it has 88 characters (the filepath on github has to have less than 88 characters of course, this seems like a safe assumption).

I've made some formatting changes to netCDF4_.py to pass the pyflake tests. I've moved the import statement for LooseVersion to the start of the script.

Thanks for the reviews and advice @shoyer

[shoyer]

It looks like you accidentally kept your changes in this file?

[braaannigan]

I thought that I had checked out the original version again to replace all of my edits, but that doesn't seem to have worked. I'm looking for a solution.

[shoyer]

You can simply revert this file (e.g., copy/paste from master) and then add another commit

[braaannigan]

I was worried that would cause some new unforeseen problem. I've commited this now.

[shoyer]

Merged in 47c3b62.

Thanks!