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.

Sign up for GitHub

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

Jump to bottom

Install locales package before running locale-gen #864

Merged
merged 1 commit into from
Sep 15, 2020
Merged

Install locales package before running locale-gen #864

merged 1 commit into from
Sep 15, 2020

Conversation

amacneil
Copy link
Contributor

@amacneil amacneil commented Sep 4, 2020

This is required if running in ubuntu docker container.

clalancette
clalancette reviewed Sep 8, 2020
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

This seems good to me. Can you please also add it to:

Thanks.

@amacneil amacneil force-pushed the patch-1 branch 3 times, most recently from 6d6efb3 to 72c38ed Compare September 9, 2020 20:05
@amacneil
Copy link
Contributor Author

amacneil commented Sep 9, 2020

@clalancette updated, I also tidied things up and moved the instructions to an include.

clalancette
clalancette requested changes Sep 14, 2020
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Two more small things to fix, then this will be good to go. Thanks for iterating.

source/Installation/Crystal/Linux-Development-Setup.rst Outdated
sudo locale-gen en_US en_US.UTF-8
sudo update-locale LC_ALL=en_US.UTF-8 LANG=en_US.UTF-8
export LANG=en_US.UTF-8
.. include:: ../_Set-Locale.rst
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not change the Crystal instructions. It is no longer supported, so I don't think we should be updating the instructions for it.

source/Installation/Dashing/Linux-Development-Setup.rst Outdated
sudo locale-gen en_US en_US.UTF-8
sudo update-locale LC_ALL=en_US.UTF-8 LANG=en_US.UTF-8
export LANG=en_US.UTF-8
.. include:: ../_Set-Locale.rst
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the use of an include. Can you please rename it to _Linux-Set-Locale.rst, since it is Linux-specific?

@amacneil
Install locales package before running locale-gen
13854df 
This is required before running locale-gen if running in ubuntu or debian
docker container.

Refactored duplicate instructions into include.
@amacneil
Copy link
Contributor Author

@clalancette updated.

Note with the include naming convention I was following the example set with _Apt-Repositories.rst. You might want to rename that to _Linux-Apt-Repositories.rst too.

@clalancette
Copy link
Contributor

@clalancette updated.

Thanks, appreciated.

Note with the include naming convention I was following the example set with _Apt-Repositories.rst. You might want to rename that to _Linux-Apt-Repositories.rst too.

Yeah, good call. That should be renamed.

@clalancette clalancette merged commit 60de973 into ros2:master Sep 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clalancette clalancette clalancette approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@amacneil @clalancette