-
Notifications
You must be signed in to change notification settings - Fork 59
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
Fix windows tests #1104
Fix windows tests #1104
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1104 +/- ##
========================================
Coverage 81.79% 81.79%
========================================
Files 25 25
Lines 5212 5223 +11
Branches 1255 1256 +1
========================================
+ Hits 4263 4272 +9
- Misses 642 644 +2
Partials 307 307 ☔ View full report in Codecov by Sentry. |
f0e29c8
to
32c0676
Compare
@benjwadams the nc-config bash script is working now on Windows. The error now is probably upstream with nczarr's implementation on Windows:
Edit: I was able to get this to work on my Windows machine. Not sure what are the differences here. Investigating... It is quite frustrating! |
4f942e5
to
42db7a3
Compare
@benjwadams this one is ready for review. While nc-config is working on Windows now, nczarr is not. I'm debugging this and will open an issue upstream when ready. I'm pretty sure it is unrelated to compliance-checker. |
61d9cc7
to
d762a0c
Compare
|
||
def _fix_windows_slashes(zarr_url): | ||
if platform.system() == "Windows": | ||
zarr_url = zarr_url.replace("///", "//") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one had me chasing red herrings all over the code base. I'm not sure if nczarr has a bug, or Python as_uri, or something else. All I know is that we can't have 3 / on Windows, only 2!
on_windows = platform.system() == "Windows" | ||
|
||
if on_windows: | ||
ncconfig = ["sh", f"{os.environ['CONDA_PREFIX']}\\Library\\bin\\nc-config"] | ||
else: | ||
ncconfig = ["nc-config"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benjwadams while this works, I'm not sure we need this. We can find the version with netCDF4 and let the zarr availability be checked by the failing test. What do you think?
d762a0c
to
3213f6d
Compare
@benjwadams I'd like to go back to #1094 but it would be nice to get this one in first to avoid big conflicts. Do you think you'll have some time to check this one? It is mostly adding Windows tests and a small fix. |
This should fix the detection of nczarr on Windows but will work only on CIs with bash and conda. I guess that is our default environment for developing on Windows anyway. I'm looking into upstream a non-bash script to libnetcdf so we can drop these workarounds.
Closes #1102