-
Notifications
You must be signed in to change notification settings - Fork 262
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
CMake: Use helper libraries for nczarr tests #2783
Conversation
I am surprised the speed up is as much as you say. IMO the files are not very big. |
Avoids rebuilding the same files multiple times
09c8ade
to
0b9c994
Compare
Yes, me too! I suspect it's actually mostly from not printing the warnings many times rather than a significant build speed up |
Great change, thanks! I'm working through all these great PR's we've seen. As backlogs go, I'm happier to have these improvements than not. Thanks! -Ward |
There does appear to be an issue under Visual Studio, but I'll take a look at that and push a fix if required. |
The import/export mechanic required by dynamic shared libraries on Windows is complicating this a fair bit; the common helper library mechanism introduced is (as exists in the PR) insufficient, insofar as there are unresolved symbols when linking via Visual Studio. I believe I have a fix, or have at the very least will have replaced these issues with other, new ones. I'll have something in soon. |
Ah, didn't even think about that! Would making it |
Setting |
There was also some fiddling with setting |
Speaking of, thanks for this and all the other PR's you've put up recently. They'll all have to be looked at under similar lenses, and I know it's taking a bit of time to get to them, but they're sitting there staring at me where I can't accidentally forget about them XD. Lots to do, I'm sure you understand. |
Yep, no worries -- netCDF is pretty crucial in our field, so I appreciate all your work, and I'm just happy to be able to give something back! As a heads up, I'm intending to get netCDF to zero warnings over the next month or so, so there are more PRs incoming I'm afraid! I figured you would rather lots of smaller PRs than one big enormous one 😄 |
Sounds good; just bear in mind we support Windows (Visual Studio), 32-bit and 64-bit hardware, and Big Endian machines as well as Little. All that is to say, sometimes the warnings are a bit trickier than they initially appear :). |
Avoids rebuilding the same files multiple times. This cuts down on duplicated warnings, and gives a little bit of a speedup to the build (~5-10%)