-
Notifications
You must be signed in to change notification settings - Fork 216
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
PROJ 6.2+ required; remove global context; autoclose database #412
Conversation
1fb9e89
to
be3c00c
Compare
Intermittent core dumping:
|
be3c00c
to
4551274
Compare
7db1ca6
to
b90650f
Compare
Core dumping still an issue. |
Core dumping does not seem to be an issue with the most recent change. |
50294c3
to
0b3b3f0
Compare
83589c0
to
19a3c95
Compare
I built
|
Thanks @cgohlke! Sounds like I need to try a different approach. |
@cgohlke mind trying out this iteration? |
This iteration segfaults during |
So, is it something that happens sometimes but not always? Or is it pretty regular? |
Just pushed another iteration to try. It's quite the guessing game what the problem could be. |
Interesting ...
|
Okay, should be stable now. |
🤞 |
Unfortunately it still crashes during test_datum.
Tests always crash at the same point when Heap Verification is on. When it's turned off, the test most often finish with many failures:
|
@cgohlke thanks for running it again! The test failure output is very useful, so thanks for sharing. Sounds like I need to try something different. |
It seems like may will have to wait for 6.2.1:
Many of the attempts I have tried have led to dead ends: Use the context of the parent object:
Create temporary context:
Use the global (NULL) context for children:
|
74858af
to
503060d
Compare
503060d
to
1bbb0b6
Compare
|
Thanks for running it again. Sounds like a step in the right direction with more adjustments needed. Mind sharing where the crashes occur? |
I think the main difference is that I link to libproj statically. In fact, switching to dynamic linking previously solved some issues. See #403 (comment)
See #403 (comment)
|
This part was super helpful:
I removed all instances where I set the variable to NULL after destroying it. Mind trying again @cgohlke? |
Actually, this might just be the debugger positioning the cursor after the instruction that crashes. The crash is in |
Ha, oh well. Okay, I have another idea I plan to try out later. |
Alright, next iteration ready. @cgohlke if you wouldn't mind trying it out, I would appreciate it. Thanks! |
The crashes are gone! Some test failures remain:
|
Fantastic! Alright, I will push another change in a bit that will hopefully get us another step closer. |
@cgohlke, just pushed another update. |
@cgohlke, do you happen to have The reason I ask is the tests expect it to not be there. This is the full warning expected:
|
Yes. Should that file not be distributed with pyproj? I added it because project monitor reported the file failed to open. I removed the file an rebuilt the latest remove_global_context branch: all tests pass, no crash!
|
That is great news!
That's fine to distribute it with it. I just need to update the tests to be a little more defensive about it's existence. Probably good to have it documented somewhere for reference what files are included in the windows wheels. Maybe in the installation instructions. Mind letting me know what datumgrid files you include in the wheel? |
Thanks again for your help @cgohlke! |
@snowman2 is this something that can be done on one of the appveyor builds as well? (just a question, totally unfamiliar with this aspect) |
It would be nice to have. I am not familiar with static linking on Windows/MSVC, so it would require some research to get it done. |
Fully understand that :) (and I am also not the one going to do it, just making suggestions) Maybe open an issue for it? |
history.rst
for all changes andapi/*.rst
for new API