Local language support (LC_ALL) added to installation& launcher on macOS #1881

random1717 · 2018-06-22T14:44:33Z

Requirements

[* ] Is there a issue open describing the motivation for this pull request ?
Does the pull request need an update of the documentation ?
[ *] Does the pull request need additional test(s) ?
Tested on macOS
Clean up the warnings generated by the style guide

Description of the Change

Adding support for non-English language local Unicode during the installation.
Please note that LC_ALL overwrites LANG

Steps and Constraints

install_sct updated

Applicable Issues

Implements or Fixes #1880 and #1851 (duplicate)

perone · 2018-06-22T14:51:45Z

We talked about that in Slack before, I don't think it's a good solution because it will be forcing English locale into users computers. It will instruct internationalized programs to use the English language even if their language isn't English.

tanguyduval · 2018-06-22T14:51:57Z

@abtahizadeh the error appears during the installation (before writting/sourcing .bashrc).
you need to call export LC_ALL=en_US.UTF-8 in addition to writing it to bashrc

random1717 · 2018-06-22T15:01:54Z

@perone Didn't know about your previous discussion, I don't think there's other solution than this ... what should we do?

@tanguyduval thanks! update in progess

zougloub · 2018-06-22T15:02:30Z

Setting the en_US.UTF-8 locale globally in the bashrc is not the proper solution. A more localized fix is needed. The closest I could think of was in the sct_launcher (no contamination as SCT is not localized). Or maybe detecting the situation in the installer and printing a big fat warning sign at the end, but nobody would process the information :)

random1717 · 2018-06-22T15:40:29Z

@zougloub normally, $LC_ALL is not set by default in macOS, which I guess wouldn't hurt to modify it. I agree with taking an action on sct_launcher, printing warning might not be enough IMO.

…e impact and scope

random1717 · 2018-06-22T16:06:05Z

Moving the solution to sct_launcher

zougloub

Looks good but I don't have a Mac.

random1717 · 2018-06-22T16:38:10Z

Works on my MAC, @tanguyduval can you please verify?

jcohenadad

Since the crash occurred during installation (tanguy, charley can you confirm?) i think we also need to take action inside install_sct, right?

tanguyduval · 2018-06-23T09:42:45Z

Right, crash appears during installation. Lc_all should be define at the begining of install_sct also. I cannot check this Pr because I have no more access to a foreign computer. Le ven. 22 juin 2018 à 20:37, Julien Cohen-Adad <[email protected]> a écrit :

…

***@***.**** requested changes on this pull request. Since the crash occurred during installation (tanguy, charley can you confirm?) i think we also need to take action inside install_sct, right? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1881 (review)>, or mute the thread <https:/notifications/unsubscribe-auth/AHbLZF6hyjMJDHJJY_t9gDlFjOlCZbJKks5t_TligaJpZM4Uz7GA> .

random1717 · 2018-06-23T13:40:00Z

@jcohenadad done
@tanguyduval Thanks for the tips
@zougloub or @perone Can you plz approve?

Can't approve on something that modifies a user's general settings.

jcohenadad · 2018-06-24T00:55:28Z

install_sct

+
+ # Fix for non-English Unicode systems on MAC
+ if [ -z ${LC_ALL} ]; then
+ echo "export LC_ALL=en_US.UTF-8" >> ~/.bash_profile


i though we agreed to not modify the bash_profile? how about updating the env variable during install (to make TF happy), and also modify it in the launcher (to have it run during execution of TF-related functions)?

done
I am not able to test, struggling with some problems related to pip ...

it should be tested before merging, esp. on a laptop where the error UTF8-related occurs

@abtahizadeh changing local language to reproduce the error is extremely easy and takes 5s. Explanation here: microsoft/vscode#7301

…hem to launcher

jcohenadad · 2018-06-24T02:16:10Z

install_sct

@@ -242,11 +242,11 @@ if uname -a | grep -i darwin > /dev/null 2>&1; then

 # Fix for non-English Unicode systems on MAC
 if [ -z ${LC_ALL} ]; then
- echo "export LC_ALL=en_US.UTF-8" >> ~/.bash_profile
+ export LC_ALL=en_US.UTF-8


not going to work with (t)csh.

Can it enter this if statement in Linux system? (If not, then disregard my comment about csh)

We have an IF statement to check, we execute only on macOS:
if uname -a | grep -i darwin > /dev/null 2>&1; then

sorry the previous lines were hidden by github. All good then 👍

@jcohenadad plz approve

@abtahizadeh as mentioned in #1881 (comment), this PR needs to be tested before merging. You mentioned you had issues with pip that you will be fixing.

random1717 · 2018-06-24T02:25:57Z

No, right now I can't test on my laptop, got some broken stuff related to pip, need to fix them first.
I thought the error is occurring only on mac (locale.getdefaultlocale() fails on Mac OS X with default language set to English #1851)?

jcohenadad · 2018-07-03T17:36:41Z

tested ea6ef08 on an OSX Terminal configured with Japanese encoding. Installed passed successfully, however sct_testing behaves strangely (see messages) and got stuck at sct_get_centerline:

sct_testing -d 1

--
Spinal Cord Toolbox (feature/1880/ea6ef085316110fc0361c82525985b19af1dbcb6)
Running /Users/julien/code/sct/scripts/sct_testing.py -d 1

Downloading testing data...

Trying URL: https://osf.io/z8gaj/?action=download
Downloading 20180125_sct_testing_data.zip...
Status: 100%|##############################| 6.78M/6.78M [00:04<00:00, 1.63MB/s]

Check if folder already exists...
WARNING: Folder sct_testing_data already exists. Removing it...
rm -rf sct_testing_data

Unzip data to: /Users/julien

Remove temporary file...
Done!


Path to testing data: /Users/julien/sct_testing_data
Checking sct_analyze_lesion.........................[OK]
Checking sct_analyze_texture........................[OK]
Checking sct_apply_transfo..........................[OK]
Checking sct_compute_ernst_angle....................[OK]
Checking sct_compute_hausdorff_distance.............The process has forked and you cannot use this CoreFoundation functionality safely. You MUST exec().
Break on __THE_PROCESS_HAS_FORKED_AND_YOU_CANNOT_USE_THIS_COREFOUNDATION_FUNCTIONALITY___YOU_MUST_EXEC__() to debug.
The process has forked and you cannot use this CoreFoundation functionality safely. You MUST exec().
Break on __THE_PROCESS_HAS_FORKED_AND_YOU_CANNOT_USE_THIS_COREFOUNDATION_FUNCTIONALITY___YOU_MUST_EXEC__() to debug.
[OK]
Checking sct_compute_mtr............................[OK]
Checking sct_compute_mscc...........................[OK]
Checking sct_compute_snr............................[OK]
Checking sct_concat_transfo.........................[OK]
Checking sct_convert................................[OK]
Checking sct_create_mask............................The process has forked and you cannot use this CoreFoundation functionality safely. You MUST exec().
Break on __THE_PROCESS_HAS_FORKED_AND_YOU_CANNOT_USE_THIS_COREFOUNDATION_FUNCTIONALITY___YOU_MUST_EXEC__() to debug.
The process has forked and you cannot use this CoreFoundation functionality safely. You MUST exec().
Break on __THE_PROCESS_HAS_FORKED_AND_YOU_CANNOT_USE_THIS_COREFOUNDATION_FUNCTIONALITY___YOU_MUST_EXEC__() to debug.
[OK]
Checking sct_crop_image.............................[OK]
Checking sct_dice_coefficient.......................[OK]
Checking sct_deepseg_gm.............................[OK]
Checking sct_deepseg_sc.............................[OK]
Checking sct_detect_pmj.............................[OK]
Checking sct_dmri_compute_dti.......................[OK]
Checking sct_dmri_concat_bvals......................[OK]
Checking sct_dmri_concat_bvecs......................[OK]
Checking sct_dmri_create_noisemask..................[OK]
Checking sct_dmri_compute_bvalue....................[OK]
Checking sct_dmri_moco..............................[OK]
Checking sct_dmri_separate_b0_and_dwi...............[OK]
Checking sct_dmri_transpose_bvecs...................[OK]
Checking sct_extract_metric.........................[OK]
Checking sct_fmri_compute_tsnr......................[OK]
Checking sct_fmri_moco..............................[OK]
Checking sct_get_centerline.........................

UPDATE 2018-07-03 14:04: The issue with sct_testingis not specific to this PR and also appears on master. Issue opened here: #1898

random1717 · 2018-07-03T18:36:00Z

Tested on manufactured Sucheng (Chinese) macOS, installation successful,
got stuck at sct_deepseg_gm (!) when running sct_testing -d 1 ...

perone · 2018-07-03T19:42:58Z

@abtahizadeh is this Sucheng (Chinese) macOS running inside a VM or something like that ? The sct_deepseg_gm can be very slow if the VM is running without virtualization extensions. Did you check if the CPU is idle when the sct_deepseg_gm is stuck ? It might happen that it is just being slow.

random1717 · 2018-07-03T19:49:10Z

@perone Yes, VM, and I agree that might be because of not having enough resources, I also suspected that it might be because of running out of RAM. I am currently running another test to see if it works with 16G RAM and 4 CPUs.

random1717 · 2018-07-03T20:56:22Z

Test passed, the issue was related to insufficient memory/CPU, however, this bug still introduced: #1900

Local language support (LC_ALL) added to installation& launcher on macOS Former-commit-id: e574232

Local language added to .bash_profile on macOS, fixes 1880

b9a1886

random1717 requested review from tanguyduval and jcohenadad June 22, 2018 14:46

Setting LC_ALL env var in macOS moved to sct_launcher to mininmize th…

ab94593

…e impact and scope

random1717 requested a review from zougloub June 22, 2018 16:12

zougloub previously approved these changes Jun 22, 2018

View reviewed changes

jcohenadad requested changes Jun 22, 2018

View reviewed changes

Fix for LC_ALL env var, moveing solution to install_sct

6182307

random1717 requested a review from perone June 23, 2018 13:40

random1717 added the installation category: install_sct or pip/setup.py label Jun 23, 2018

jcohenadad reviewed Jun 24, 2018

View reviewed changes

Modifying install_sct to set ENV variables LC_ALL and LANG + adding t…

47efcb9

…hem to launcher

random1717 changed the title ~~Local language support (LC_ALL) added to .bash_profile on macOS during the installation~~ Local language support (LC_ALL) added to installation& launcher on macOS Jun 24, 2018

jcohenadad reviewed Jun 24, 2018

View reviewed changes

Merge branch 'master' into feature/1880

3f7f4b4

random1717 removed request for perone and tanguyduval June 27, 2018 13:54

Merge branch 'master' into feature/1880

ea6ef08

jcohenadad added the priority:HIGH label Jul 3, 2018

Merge branch 'master' into feature/1880

38532a9

jcohenadad approved these changes Jul 4, 2018

View reviewed changes

random1717 merged commit e574232 into master Jul 4, 2018

random1717 deleted the feature/1880 branch July 4, 2018 13:19

jcohenadad added this to the v3.2.2 milestone Jul 7, 2018

jcohenadad mentioned this pull request Jul 10, 2018

locale.getdefaultlocale() fails on Mac OS X with default language set to English #1851

Closed

jcohenadad pushed a commit that referenced this pull request Dec 18, 2019

Merge pull request #1881 from neuropoly/feature/1880

9d43fd9

Local language support (LC_ALL) added to installation& launcher on macOS Former-commit-id: e574232

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Local language support (LC_ALL) added to installation& launcher on macOS #1881

Local language support (LC_ALL) added to installation& launcher on macOS #1881

random1717 commented Jun 22, 2018 •

edited

Loading

perone commented Jun 22, 2018 •

edited

Loading

tanguyduval commented Jun 22, 2018

random1717 commented Jun 22, 2018

zougloub commented Jun 22, 2018 •

edited

Loading

random1717 commented Jun 22, 2018

random1717 commented Jun 22, 2018

zougloub left a comment

random1717 commented Jun 22, 2018

jcohenadad left a comment

tanguyduval commented Jun 23, 2018 via email

random1717 commented Jun 23, 2018

jcohenadad Jun 24, 2018

random1717 Jun 24, 2018

jcohenadad Jun 24, 2018

jcohenadad Jul 3, 2018

jcohenadad Jun 24, 2018

jcohenadad Jun 24, 2018

random1717 Jun 25, 2018

jcohenadad Jun 25, 2018

random1717 Jun 25, 2018

jcohenadad Jun 25, 2018

random1717 commented Jun 24, 2018

jcohenadad commented Jul 3, 2018 •

edited

Loading

random1717 commented Jul 3, 2018

perone commented Jul 3, 2018

random1717 commented Jul 3, 2018 •

edited

Loading

random1717 commented Jul 3, 2018

Local language support (LC_ALL) added to installation& launcher on macOS #1881

Local language support (LC_ALL) added to installation& launcher on macOS #1881

Conversation

random1717 commented Jun 22, 2018 • edited Loading

Requirements

Description of the Change

Steps and Constraints

Applicable Issues

perone commented Jun 22, 2018 • edited Loading

tanguyduval commented Jun 22, 2018

random1717 commented Jun 22, 2018

zougloub commented Jun 22, 2018 • edited Loading

random1717 commented Jun 22, 2018

random1717 commented Jun 22, 2018

zougloub left a comment

Choose a reason for hiding this comment

random1717 commented Jun 22, 2018

jcohenadad left a comment

Choose a reason for hiding this comment

tanguyduval commented Jun 23, 2018 via email

random1717 commented Jun 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

random1717 commented Jun 24, 2018

jcohenadad commented Jul 3, 2018 • edited Loading

random1717 commented Jul 3, 2018

perone commented Jul 3, 2018

random1717 commented Jul 3, 2018 • edited Loading

random1717 commented Jul 3, 2018

random1717 commented Jun 22, 2018 •

edited

Loading

perone commented Jun 22, 2018 •

edited

Loading

zougloub commented Jun 22, 2018 •

edited

Loading

jcohenadad commented Jul 3, 2018 •

edited

Loading

random1717 commented Jul 3, 2018 •

edited

Loading