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

improve parallel builds, fixed SEGFAULT for NULL parameter for nc_inq_format(), added testing, documentation #680

Merged

Conversation

edhartnett
Copy link
Contributor

@edhartnett edhartnett commented Nov 22, 2017

This PR started with adding some testing for ncfunc.c, which revealed a SEGFAULT when a NULL parameter is passed to nc_inq_format(). This is fixed in this PR.

Also I continued work on autotools build system for smoother parallel builds. Many test dependencies are eliminated by ensuring that all tests use unique file names for test output. Parallel builds now work for builds with dap remote tests, and buildfiles are simplified by removal of unneeded dependencies.

Also took out some dead code and added doxygen documentation for the functions I touched.

Fixes #678.
Fixes #677.
Fixes #682.
Fixes #681.
Fixes #670.
Fixes #616.
Fixes #667.
Fixes #687.
Fixes #685.
Fixes #689.
Fixes #688.
Fixes #684.
Fixes #690.
Fixes #695.
Fixes #693.

@WardF
Copy link
Member

WardF commented Nov 22, 2017

Why did you remove tst_filters? That's part of the new code added by Dennis for multiple compression filters, I believe. @DennisHeimbigner is that correct?

@edhartnett
Copy link
Contributor Author

Sorry, didn't remove tst_filters, remove tst_formats. Just mistyped it in git commit comment. ;-)

I added tst_formats.c and am having some trouble getting it to work with travis. I also added a ref_.hdf4 file, and that seems to be part of the trouble. I will keep working it until I figure it out...

@DennisHeimbigner
Copy link
Collaborator

The file test_filters is part of the filter_test directory.
There is a specific ./configure flag to control if the tests
in filter_test are attempted: --enable-filter-tests
This defaults to off because this test is a bit tricky to run
automatically and as far as I recall, I was never able to get it to
run under cmake.
It looks like the equivalent option is not defined in CMakeLists.txt.
It should be and it should default to off. The relevant place it hould be used
is in nc_test4/filter_test/CMakeLists.txt.
See how it is used in nc_test4/filter_test/Makefile.am.

@edhartnett
Copy link
Contributor Author

OK, this is now working for both autotools and cmake builds.

As noted above, I did NOT remove tst_filters, just mistyped "tst_formats" in a commit comment. No tests were harmed in the production of this pull request.

The new tst_foramats is added to test nc_inq_foramt() and nc_inq_format_extended().

@edhartnett edhartnett changed the title fixed SEGFAULT for NULL parameter for nc_inq_format(), added testing, documentation improve parallel builds, fixed SEGFAULT for NULL parameter for nc_inq_format(), added testing, documentation Nov 26, 2017
@WardF
Copy link
Member

WardF commented Nov 28, 2017

Evaluating now, thanks!

@WardF WardF self-assigned this Nov 28, 2017
@WardF WardF added this to the 4.5.1 milestone Nov 28, 2017
@WardF
Copy link
Member

WardF commented Nov 28, 2017

CMake configuration breaks using my automated scripts on Windows, but probably something basic. I'll investigate.

@edhartnett
Copy link
Contributor Author

OK, keep me in the loop. I am happy to revert some files to eliminate the problem and get the rest of the changes through, then I can circle back around to the problem area.

Unfortunately windows is the one giant gap in my CI. I don't have a windows machine with visual studio. Not sure if there is a free version I can use or whether I need to buy one...

@WardF
Copy link
Member

WardF commented Nov 28, 2017

The issue is only on Windows, so I'm sure it's pretty straightforward; I'll outline what I find and if it's something I can't fix myself. First avenue of investigation is my automated tools to see if they're doing something they shouldn't, now.

@DennisHeimbigner
Copy link
Collaborator

Ed, FYI the community edition of Visual Studio is free.

@edhartnett
Copy link
Contributor Author

I have found an old windows laptop. I will fire it up and see what I can manage wrt visual studio community edition. ;-)

@WardF
Copy link
Member

WardF commented Nov 29, 2017

No prob I had to set this aside suddenly yesterday (son sick at school) but sitting down to it now:)

@WardF
Copy link
Member

WardF commented Nov 29, 2017

I found the issue, it's no big deal, when MSVC is true a property is being set that doesn't need to be set. This is from some older cmake code that could do with cleaning up; cmake has come a long way since we started using it, plus I can refine a lot of what's been done. I'll take care of it and get this merged, thanks!

@WardF
Copy link
Member

WardF commented Nov 29, 2017

Also FWIW I do my debugging/testing for Windows in a Win10 VM using VirtualBox, with no ill effects.

@edhartnett
Copy link
Contributor Author

edhartnett commented Nov 29, 2017 via email

@edhartnett
Copy link
Contributor Author

@WardF hope your son is feeling better. ;-(

Meanwhile I have been working on PIO issues all day so still have not yet set up a test environment...

@WardF
Copy link
Member

WardF commented Nov 29, 2017

Running mpich parallel tests and then will merge. @edhartnett one thing our (docker-based) regression testing does is it tests the Fortran, C++ and Python interfaces, as well as NCO (for non parallel builds) and parallel-netcdf (for parallel builds) against the particular C library being generated. I don't know if that's something your Jenkins build is set up to do, and to be honestly it only rarely finds an issue, but it is pretty handy when it does.

Thanks!

@WardF WardF merged commit f69e031 into Unidata:master Nov 29, 2017
@edhartnett edhartnett deleted the ejh_ncfunc_testing branch December 20, 2017 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment