Skip to content

Hiding all IPhreeqc symbols by default from our custom static library#424

Merged
rkingsbury merged 1 commit into
mainfrom
vb/pr397_fix
Jun 16, 2026
Merged

Hiding all IPhreeqc symbols by default from our custom static library#424
rkingsbury merged 1 commit into
mainfrom
vb/pr397_fix

Conversation

@vineetbansal

@vineetbansal vineetbansal commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator

I'm pretty sure all the segfaults and hanging-up that we're seeing in PR #397 is because the from pyEQL.phreeqc import PHRQSol has been moved to the module level, meaning it runs unconditionally.

While this in itself should not be a problem at the python level, the fact that we're seeing segfaults means that our C++ extension is clashing with the Phreeqc's extension - both our library and PhreeqcPython are making the same symbols available in the python process, and since we differ in our mutual versions of IPhreeqc that we depend on, things are getting weird. I'm not sure why this would only happen on a Mac, but that's an operating system nuance that we can take as a given.

In other words, both our extension and PhreeqcPython are being sloppy in hiding stuff in IPhreeqc that we haven't provided wrappers for (and should thus remain hidden). This PR fixes that from our end.

We could ask Shaun to move the import where it is actually used instead of globally to get around this, but really he (or other python programmers) shouldn't have to worry about this kind of stuff.

Some more details are here. This is all conjecture, but this change shouldn't hurt anything, and is good hygiene for us anyway. It will be interesting to see if PR #397 starts to work after these changes are merged.

@codecov

codecov Bot commented Jun 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.45%. Comparing base (96806f9) to head (bb07cdc).
⚠️ Report is 14 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #424   +/-   ##
=======================================
  Coverage   86.45%   86.45%           
=======================================
  Files          14       14           
  Lines        1912     1912           
  Branches      336      336           
=======================================
  Hits         1653     1653           
  Misses        210      210           
  Partials       49       49           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@vineetbansal vineetbansal requested a review from rkingsbury June 16, 2026 13:05
@rkingsbury rkingsbury merged commit 748a5d4 into main Jun 16, 2026
16 of 17 checks passed
@rkingsbury

Copy link
Copy Markdown
Member

Thanks @vineetbansal ; I would never have thought of this!

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