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

Set locale before (de)serializing data for SQLite #1837

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bennibbelink
Copy link
Contributor

Closes cyclus/cymetric#205.

I think there must be some inconsistency with how libc++ and libboost handle default locales. This PR calls ss.imbue() to set the locale according to the user's environment, which seems to solve the segfault describe in cyclus/cymetric#205 (at least when I build and test on my arm Mac machine). We could hardcode this to the "C" locale if we want to make it more robust. The instance where I could see issues arising is if a user runs a sim in one environment then reads the SQLite DB in a different environment with a different locale.

@bennibbelink bennibbelink marked this pull request as ready for review December 9, 2024 20:34
@bennibbelink bennibbelink requested a review from gonuke December 9, 2024 20:34
@coveralls
Copy link
Collaborator

coveralls commented Dec 9, 2024

Pull Request Test Coverage Report for Build 12243810876

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 41.118%

Totals Coverage Status
Change from base Build 12120029665: 0.0%
Covered Lines: 51664
Relevant Lines: 125647

💛 - Coveralls

Copy link

github-actions bot commented Dec 9, 2024

Downstream Build Status Report - b4d8e8b - 2024-12-09 20:33:08 +0000

Build FROM cyclus_20.04_apt/cyclus
  • Cycamore: Success
  • Cymetric: Success
Build FROM cyclus_20.04_conda/cyclus
  • Cycamore: Success
  • Cymetric: Success
Build FROM cyclus_22.04_apt/cyclus
  • Cycamore: Success
  • Cymetric: Success
Build FROM cyclus_22.04_conda/cyclus
  • Cycamore: Success
  • Cymetric: Success
Build FROM cyclus_24.04_apt/cyclus
  • Cycamore: Success
  • Cymetric: Success
Build FROM cyclus_24.04_conda/cyclus
  • Cycamore: Success
  • Cymetric: Success

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we decided to explore the idea of selecting a locale and storing it in the DB so that it could be read for post-processing? Or did we decide to leave this for a future feature?

@bennibbelink
Copy link
Contributor Author

I thought we decided to store it in the DB. That being said however, I'm struggling with the implementation because the place that we need to read the locale from the DB is in the code interfacing with the DB.

I'm thinking we read the locale from the environment, falling back to the C locale if not specified. This leaves us with the potential incompatibility of users writing archetypes named using non-ASCII characters. I can try to add tests for this and appropriate error handling so it gracefully fails in this case.

@bennibbelink bennibbelink marked this pull request as draft December 11, 2024 23:31
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.

test_inventories_compact() fails with Sqlite on arm Mac
3 participants