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

Add locale requirements to linux build script #3934

Closed
wants to merge 1 commit into from

Conversation

ksomml
Copy link

@ksomml ksomml commented Oct 4, 2024

Fixes issue #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)

  • no need to update because the change only affects the build script

This change is Reviewable

@ksomml ksomml changed the title Add locale requirements Add locale requirements to linux build script Oct 4, 2024
@halleysfifthinc
Copy link

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
Copy link
Author

ksomml commented Oct 4, 2024

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
Copy link
Contributor

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
Copy link
Author

ksomml commented Oct 7, 2024

Then I believe this pull request can be closed? Though the issue #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
Copy link
Member

aymanhab commented Oct 7, 2024

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

@aymanhab aymanhab closed this Oct 7, 2024
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.

4 participants