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

Ncdump fails under visual studio for certain dap urls #366

Merged
merged 8 commits into from
Feb 23, 2017
Merged

Conversation

DennisHeimbigner
Copy link
Collaborator

re: #365
Following command fails under visual studio.
ncdump -h http://thredds.ucar.edu/thredds/dodsC/nexrad/composite/gini/n0r/1km/20170216/Level3_Composite_n0r_1km_20170216_1635.gini

The problem is that sscanf for windows does not appear to support
scanning 8bit integers: it appears to only allow scanning of characters.

Solution:
Scan the input as an integer (for type Byte) or unsigned int (for type UByte)
and then recast the result as a char or unsigned char.

Primary code fix is in libdap2/dapcvt.c#dapcvtattrval

Following command fails under visual studio.
    ncdump -h http://thredds.ucar.edu/thredds/dodsC/nexrad/composite/gini/n0r/1km/20170216/Level3_Composite_n0r_1km_20170216_1635.gini

The problem is that sscanf for windows does not appear to support
scanning 8bit integers: it appears to only allow scanning of characters.

Solution:
Scan the input as an integer (for type Byte) or unsigned int (for type UByte)
and then recast the result as a char or unsigned char.

Primary code fix is in libdap2/dapcvt.c#dapcvtattrval
re: #365

1. Added to RELEASENOTES.md
2. Add a range check to more closely
   mimic unix sscanf
3. locate and fix same sscanf problems in ncgen/cvt.c

Still need a stable url for a test case.
@WardF
Copy link
Member

WardF commented Feb 21, 2017

Encountering build failures reported in ncgen.l when -DENABLE_HDF4=TRUE. Details can be seen here:

Failures not occuring when HDF4 support is disabled. Debugging now, if anything about these errors leaps out, let me know.

@WardF
Copy link
Member

WardF commented Feb 21, 2017

Looks like I found the compilation issue at least; ncgen.y was defining a variable of name constant, which is reserved with Visual Studio. renamed, rebuilding to run tests now.

@WardF
Copy link
Member

WardF commented Feb 21, 2017

Ok, compilation issue is not resolved, and not certain where it was introduced. Will update here when ready to merge. It looks like it may pre-date this branch.

@DennisHeimbigner
Copy link
Collaborator Author

Looking at a number of the failed tests here,
http://my.cdash.org/viewTest.php?onlyfailed&buildid=1139214
Some are because a previous call to e.g. ncdump failed and did not create a file
and some (tst_inmemory_nc3) are because a call to rm -fr result failed
to remove all the files in the directory -- permissions problem?
might be useful to temporarily add an ls -lR command to see what is in results
and what its permissions are.

@WardF
Copy link
Member

WardF commented Feb 21, 2017

It isn't a permissions problem, as far as I can tell. The failure only occurs when HDF4 support is turned on, with failures in the ncgen generated files. Still working to figure out what the issue is.

@WardF
Copy link
Member

WardF commented Feb 21, 2017

@DennisHeimbigner To rule out something environmental, can you see if the same occurs on your system?

@DennisHeimbigner
Copy link
Collaborator Author

DennisHeimbigner commented Feb 21, 2017 via email

@WardF
Copy link
Member

WardF commented Feb 21, 2017

I've confirmed that it still works on the system under 4.4.1.1, so that tentatively rules out environmental. I'll start walking through and looking for where this broke

@WardF
Copy link
Member

WardF commented Feb 22, 2017

Ok, I've solved the issue and will be pulling all the threads together to get this and the other pulls merged.

@DennisHeimbigner maybe you can shed some light on what's going on. The issue was in the bison-generated file ncgen/ncgeny.h. When generated on Windows, it is missing a number of defines. When generated on OSX, it contains the required defines.

The odd thing about this is that, surprisingly, the version of bison on my Windows VM is newer than the OSX version. Version 2.3 on OSX, version 2.4.2 on Windows. The file is generated with the following command:

$ bison -pncg -t -d ncgen.y

There are other commands to move the generated file into place; you can see all of these near the bottom of ncgen/CMakeLists.txt. I've attached a zip file containing the two generated .h files, with version number in the filename. I don't know bison, can you provide any insight into what is going on?

bison_files.zip

@DennisHeimbigner
Copy link
Collaborator Author

Under what circumstances are ncgeny.* and ncgenl.* being recreated by ctest or travis?
I was not aware that bison could compile under cmake/Visual-Studio.
The short term answer is to make it clear that

  1. the lex and parse files should not be generated on the fly except by Unidata.
  2. the generated files ncgeny.* and ncgenl.* are a fixed part of the distribution
    and the repository and are never deleted.

@WardF
Copy link
Member

WardF commented Feb 22, 2017

They are not automatically recreated, and are tracked in github, although they can be generated manually by running 'make makeparser'. Occasionally there have been bugs which require editing something in the source files which then require the corresponding files be re-generated, which is what happened at some point in this case. Windows/Visual studio arent involved in generating them on Windows; if cmake can find bison on the path, it will use it l,otherwise it will throw an error or warning.

It appears that we need to regenerate these files either on OS X/Linux, on the rare occasion they must be regenerated, or possibly with an older version of bison. If it's the latter, I am concerned that our build process will break in the future. I need to install a newer version under Linux and see if the results look like what we see on windows (broken) or OS X (works)

@WardF
Copy link
Member

WardF commented Feb 23, 2017

So it does look like the issue might be the newer bison. The files you generated and committed with the newer version lack the various #define statements, as does the ncgeny.h file generated under linux with bison 3.0.4. I'll play around with it, but looking at the bison man page, I don't see any obvious changes to how bison is being invoked.

I'm going to close out this issue once the tests pass and I merge it in; this probably merits its own issue.

@DennisHeimbigner
Copy link
Collaborator Author

DennisHeimbigner commented Feb 23, 2017 via email

@DennisHeimbigner
Copy link
Collaborator Author

I was trying to install hdf4 and noticed that it has its own old version of ncgen. It is possible
that the wrong ncgen is being invoked?

@DennisHeimbigner
Copy link
Collaborator Author

Also, I note that the #defines for tokens is now an enum. Could that be causing problems?

@WardF
Copy link
Member

WardF commented Feb 23, 2017

Looking at previous version it was both an enum and a block of defined. The problem is that without the block of defines, we end up with a lot of undefined values when compiling but only with hdf4 turned on for some reason.

@WardF
Copy link
Member

WardF commented Feb 23, 2017

Also the issue isn't the invocation; Ncgen just fails to compile. I wonder if it's related to the hdf4 versions although that doesn't explain why it used to work until a bison update

@DennisHeimbigner
Copy link
Collaborator Author

BTW I note that hdf4 has a --disable-netcdf configure flag. Was that set when you build hdf4?

@WardF
Copy link
Member

WardF commented Feb 23, 2017

Yes.

@DennisHeimbigner
Copy link
Collaborator Author

DennisHeimbigner commented Feb 23, 2017 via email

@WardF
Copy link
Member

WardF commented Feb 23, 2017

correct. It doesn't fail when building with hdf4 on other platforms, or when building with visual studio without hdf4 support. It was something about the combination of hdf4 + visual studio, which seems super odd.

@DennisHeimbigner
Copy link
Collaborator Author

Ok. Finally able to reproduce. Hopefully I can figure out what is happening.

@WardF WardF merged commit 7cb0350 into master Feb 23, 2017
DennisHeimbigner added a commit that referenced this pull request Feb 24, 2017
However, it looks like visual studio is trying to compile
ncgenl.c on its own, which of course will fail.
So I changed the name to ncgenl.h so it would not
be compiled. This appears to fix the bison/flex problems in
#366.

What I still do not understand is why hdf4 is involved.

p.s. Visual Studio is really tenacious about its caches.
I had a devil of a time getting it to recognize my changes.
@WardF WardF deleted the issue365.dmh branch February 24, 2017 21:22
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request May 16, 2018
Upstream changes:
## 4.6.1 - March 15, 2018

* [Bug Fix] Corrected an issue which could result in a dap4 failure. See [Github #888](Unidata/netcdf-c#888) for more information.
* [Bug Fix][Enhancement] Allow `nccopy` to control output filter suppresion.  See [Github #894](Unidata/netcdf-c#894) for more information.
* [Enhancement] Reverted some new behaviors that, while in line with the netCDF specification, broke existing workflows.  See [Github #843](Unidata/netcdf-c#843) for more information.
* [Bug Fix] Improved support for CRT builds with Visual Studio, improves zlib detection in hdf5 library. See [Github #853](Unidata/netcdf-c#853) for more information.
* [Enhancement][Internal] Moved HDF4 into a distinct dispatch layer. See [Github #849](Unidata/netcdf-c#849) for more information.

## 4.6.0 - January 24, 2018
* [Enhancement] Full support for using HDF5 dynamic filters, both for reading and writing. See the file docs/filters.md.
* [Enhancement] Added an option to enable strict null-byte padding for headers; this padding was specified in the spec but was not enforced.  Enabling this option will allow you to check your files, as it will return an E_NULLPAD error.  It is possible for these files to have been written by older versions of libnetcdf.  There is no effective problem caused by this lack of null padding, so enabling these options is informational only.  The options for `configure` and `cmake` are `--enable-strict-null-byte-header-padding` and `-DENABLE_STRICT_NULL_BYTE_HEADER_PADDING`, respectively.  See [Github #657](Unidata/netcdf-c#657) for more information.
* [Enhancement] Reverted behavior/handling of out-of-range attribute values to pre-4.5.0 default. See [Github #512](Unidata/netcdf-c#512) for more information.
* [Bug] Fixed error in tst_parallel2.c. See [Github #545](Unidata/netcdf-c#545) for more information.
* [Bug] Fixed handling of corrupt files + proper offset handling for hdf5 files. See [Github #552](Unidata/netcdf-c#552) for more information.
* [Bug] Corrected a memory overflow in `tst_h_dimscales`, see [Github #511](Unidata/netcdf-c#511), [Github #505](Unidata/netcdf-c#505), [Github #363](Unidata/netcdf-c#363) and [Github #244](Unidata/netcdf-c#244) for more information.

## 4.5.0 - October 20, 2017

* Corrected an issue which could potential result in a hang while using parallel file I/O. See [Github #449](Unidata/netcdf-c#449) for more information.
* Addressed an issue with `ncdump` not properly handling dates on a 366 day calendar. See [GitHub #359](Unidata/netcdf-c#359) for more information.

### 4.5.0-rc3 - September 29, 2017

* [Update] Due to ongoing issues, native CDF5 support has been disabled by **default**.  You can use the options mentioned below (`--enable-cdf5` or `-DENABLE_CDF5=TRUE` for `configure` or `cmake`, respectively).  Just be aware that for the time being, Reading/Writing CDF5 files on 32-bit platforms may result in unexpected behavior when using extremely large variables.  For 32-bit platforms it is best to continue using `NC_FORMAT_64BIT_OFFSET`.
* [Bug] Corrected an issue where older versions of curl might fail. See [GitHub #487](Unidata/netcdf-c#487) for more information.
* [Enhancement] Added options to enable/disable `CDF5` support at configure time for autotools and cmake-based builds.  The options are `--enable/disable-cdf5` and `ENABLE_CDF5`, respectively.  See [Github #484](Unidata/netcdf-c#484) for more information.
* [Bug Fix] Corrected an issue when subsetting a netcdf3 file via `nccopy -v/-V`. See [Github #425](Unidata/netcdf-c#425) and [Github #463](Unidata/netcdf-c#463) for more information.
* [Bug Fix] Corrected `--has-dap` and `--has-dap4` output for cmake-based builds. See [GitHub #473](Unidata/netcdf-c#473) for more information.
* [Bug Fix] Corrected an issue where `NC_64BIT_DATA` files were being read incorrectly by ncdump, despite the data having been written correctly.  See [GitHub #457](Unidata/netcdf-c#457) for more information.
* [Bug Fix] Corrected a potential stack buffer overflow.  See [GitHub #450](Unidata/netcdf-c#450) for more information.

### 4.5.0-rc2 - August 7, 2017

* [Bug Fix] Addressed an issue with how cmake was implementing large file support on 32-bit systems. See [GitHub #385](Unidata/netcdf-c#385) for more information.
* [Bug Fix] Addressed an issue where ncgen would not respect keyword case. See [GitHub #310](Unidata/netcdf-c#310) for more information.

### 4.5.0-rc1 - June 5, 2017

* [Enhancement] DAP4 is now included. Since dap2 is the default for urls, dap4 must be specified by
(1) using "dap4:" as the url protocol, or
(2) appending "#protocol=dap4" to the end of the url, or
(3) appending "#dap4" to the end of the url
Note that dap4 is enabled by default but remote-testing is
disbled until the testserver situation is resolved.
* [Enhancement] The remote testing server can now be specified with the `--with-testserver` option to ./configure.
* [Enhancement] Modified netCDF4 to use ASCII for NC_CHAR.  See [Github Pull request #316](Unidata/netcdf-c#316) for more information.
* [Bug Fix] Corrected an error with how dimsizes might be read. See [Github #410](Unidata/netcdf-c#410) for more information.
* [Bug Fix] Corrected an issue where 'make check' would fail if 'make' or 'make all' had not run first.  See [Github #339](Unidata/netcdf-c#339) for more information.
* [Bug Fix] Corrected an issue on Windows with Large file tests. See [Github #385](Unidata/netcdf-c#385]) for more information.
* [Bug Fix] Corrected an issue with diskless file access, see [Pull Request #400](Unidata/netcdf-c#400) and [Pull Request #403](Unidata/netcdf-c#403) for more information.
* [Upgrade] The bash based test scripts have been upgraded to use a common test_common.sh include file that isolates build specific information.
* [Upgrade] The bash based test scripts have been upgraded to use a common test_common.sh include file that isolates build specific information.
* [Refactor] the oc2 library is no longer independent of the main netcdf-c library. For example, it now uses ncuri, nclist, and ncbytes instead of its homegrown equivalents.
* [Bug Fix] `NC_EGLOBAL` is now properly returned when attempting to set a global `_FillValue` attribute. See [GitHub #388](Unidata/netcdf-c#388) and [GitHub #389](Unidata/netcdf-c#389) for more information.
* [Bug Fix] Corrected an issue where data loss would occur when `_FillValue` was mistakenly allowed to be redefined.  See [Github #390](Unidata/netcdf-c#390), [GitHub #387](Unidata/netcdf-c#387) for more information.
* [Upgrade][Bug] Corrected an issue regarding how "orphaned" DAS attributes were handled. See [GitHub #376](Unidata/netcdf-c#376) for more information.
* [Upgrade] Update utf8proc.[ch] to use the version now maintained by the Julia Language project (https://github.com/JuliaLang/utf8proc/blob/master/LICENSE.md).
* [Bug] Addressed conversion problem with Windows sscanf.  This primarily affected some OPeNDAP URLs on Windows.  See [GitHub #365](Unidata/netcdf-c#365) and [GitHub #366](Unidata/netcdf-c#366) for more information.
* [Enhancement] Added support for HDF5 collective metadata operations when available. Patch submitted by Greg Sjaardema, see [Pull request #335](Unidata/netcdf-c#335) for more information.
* [Bug] Addressed a potential type punning issue. See [GitHub #351](Unidata/netcdf-c#351) for more information.
* [Bug] Addressed an issue where netCDF wouldn't build on Windows systems using MSVC 2012. See [GitHub #304](Unidata/netcdf-c#304) for more information.
* [Bug] Fixed an issue related to potential type punning, see [GitHub #344](Unidata/netcdf-c#344) for more information.
* [Enhancement] Incorporated an enhancement provided by Greg Sjaardema, which may improve read/write times for some complex files.  Basically, linked lists were replaced in some locations where it was safe to use an array/table.  See [Pull request #328](Unidata/netcdf-c#328) for more information.
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