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

Use canonical path to derive name of dataset on Windows #2104

Merged
merged 1 commit into from
Mar 15, 2022

Conversation

mjwoods
Copy link
Contributor

@mjwoods mjwoods commented Sep 4, 2021

If #2084 is merged , netcdf utilities will use Windows paths on mingw-w64. This leads to problems in the output from ncdump, such as the following error from dap4_test/test_meta.sh:

checking: test_anon_dim.2.syn
1c1
< netcdf test_anon_dim.2 {
---
> netcdf .\\results_test_meta\\test_anon_dim.2 {

The default name of the dataset is supposed to be the basename of the input file, minus any file extension. The existing function name_path which determines the default dataset name assumes that paths have Unix directory separators (/), but Windows paths use \\. This PR produces a compatible path format using function NCpathcanonical instead of NCpathcvt.

@mjwoods mjwoods requested a review from WardF as a code owner September 4, 2021 06:59
@DennisHeimbigner
Copy link
Collaborator

Good catch. Thanks. This might also be an issue inside the library in nc_create() and
nc_open().

@mjwoods
Copy link
Contributor Author

mjwoods commented Sep 6, 2021

I haven't seen any sign of problems in nc_create and nc_open. In my tests, after all of my current PRs (#2106, #2084, #2103, #2104, #2105 ) are merged into master, only 6 tests fail on mingw (compared with 63 for master at commit 09e0e04).

The remaining test failures are:
22 - ncdump_tst_nccopy_w3 (Failed)
24 - ncdump_tst_nccopy_w4 (Failed)
41 - ncdump_test_unicode_directory (Failed)
42 - ncdump_test_unicode_path (Failed)
67 - nc_test_tst_diskless6 (Failed)
71 - nc_test_run_inmemory (Failed)

These failures are related to unicode and mmap support. I plan to look into them later.

@WardF
Copy link
Member

WardF commented Mar 10, 2022

Rebasing and retesting to see where it's at.

@WardF WardF added this to the 4.9.0 milestone Mar 10, 2022
@WardF WardF self-assigned this Mar 10, 2022
@WardF WardF mentioned this pull request Mar 14, 2022
WardF added a commit that referenced this pull request Mar 15, 2022
@WardF WardF merged commit 5856b8e into Unidata:main Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants