search

opensim-org / opensim-core

SimTK OpenSim C++ libraries and command-line applications, and Java/Python wrapping.
https://opensim.stanford.edu
Apache License 2.0
800 stars 323 forks source link

Add locale requirements to linux build script #3934

Closed ksomml closed 1 month ago

ksomml commented 1 month ago

Fixes issue https://github.com/opensim-org/opensim-core/issues/3924

Brief summary of changes

Added locale changes into the build script so numeric formatting is correct when using TimeSeriesTable.h

Testing I've completed

Tested locally with a separate shell script. A restart after locale changes is recommended if locale changes have been applied due to different settings instead of directly using OpenSim after installation.

Looking for feedback on...

/

CHANGELOG.md (choose one)

This change is Reviewable

halleysfifthinc commented 1 month ago

I don't think this is the best long-term solution. This PR just masks the locale problem; anyone who installs a pre-built linux artifact on a system with a comma decimal locale will still encounter this issue because the locale change here is system specific/dependent and isn't fixing the root cause.

ksomml commented 1 month ago

That is true, mainly wanted to get attention to this issue, as it essentially makes you unable to load motions onto the model in the GUI. I do not know how to fix the issue at its root cause.

alexbeattie42 commented 1 month ago

There are a few problems with this change:

  1. Installing opensim should not override the system locale settings to en_US.UTF-8. This would create a very unexpected problem for everyone who does not use en_US.UTF-8 (ex. people who are not in the United States).
  2. Executing arbitrary Sudo system configuration specific commands in a build script should be avoided. If it can't the user should be well aware of what it is doing.
ksomml commented 1 month ago

Then I believe this pull request can be closed? Though the issue https://github.com/opensim-org/opensim-core/issues/3924 will still remain. I might take a look to see if i could fix it, but I believe someone more experienced is more likely to find the root cause of this. I already closed the PR for the opensim-gui now, because the root cause lies in opensim-core.

aymanhab commented 1 month ago

Agreed that build scripts shouldn't make global changes to either Locale or Path in order to avoid unintended side-effects, so closing the PR but thanks for bringing the issue to the forefront @ksomml and for the feedback @alexbeattie42 and @halleysfifthinc