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

Select install directory to build #18

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Select install directory to build #18

wants to merge 18 commits into from

Conversation

fmrico
Copy link
Contributor

@fmrico fmrico commented Apr 14, 2018

By setting the environment variable BASE_ROOT, you are able to select where to create the System directory where to install, instead of a fixed .ros_root in the root project directory.

If you do not set BASE_ROOT, the System directory where to install will be in the root project directory.

@fmrico
Copy link
Contributor Author

fmrico commented Apr 14, 2018 •
edited
Loading

appveyor check failed? :S

Everitying is building for me...

@esteve
Copy link
Owner

esteve commented Apr 19, 2018

@fmrico Thanks for the PR! Don't worry about Appveyor, I added it as a test, but I don't think I'll keep using it for the time being.

esteve
esteve requested changes Apr 24, 2018
View reviewed changes
README.md Outdated
If you want to set the install directory (instead of the project root directory), set the `BASE_ROOT` environment variable:

```
export export BASE_ROOT=/home/mihome/pepper_root/ <-- Or wherever you want
Copy link
Owner

Choose a reason for hiding this comment

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

s#export export#export#

s#/home/mihome#$HOME#

Copy link
Owner

Choose a reason for hiding this comment

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

Also, it'd be better to prefix this variable with PEPPER_ or PEPPER_ROS_ to avoid any conflicts, BASE_ROOT is too generic

build_ros1.sh Outdated
@@ -5,7 +5,8 @@ PYTHON2_PATCH_VERSION=13

PYTHON2_VERSION=${PYTHON2_MAJOR_VERSION}.${PYTHON2_MINOR_VERSION}.${PYTHON2_PATCH_VERSION}

INSTALL_ROOT=.ros-root
HOST_INSTALL_ROOT="${BASE_ROOT:-${PWD}}/"System
Copy link
Owner

Choose a reason for hiding this comment

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

HOST_INSTALL_ROOT="${BASE_ROOT:-${PWD}}/System"

install.sh Outdated
if [ ! -d "$(pwd)/.ros-root" ]; then
echo "ERROR: .ros-root directory is not found" 1>&2
if [ ! -d "${HOST_INSTALL_ROOT}" ]; then
echo "ERROR: System directory is not found" 1>&2
Copy link
Owner

Choose a reason for hiding this comment

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

System directory not found

install.sh Outdated
@@ -29,9 +31,9 @@ if [ -z "$master_ip" ]; then
master_ip=$local_ip
fi
# Set ROS Master URI to ip of which ros is installed (this most probably will te the ros master)
copy_script="sed -i.bak 's/^\(export\s*ROS_MASTER_URI\s*=\s*\).*$/\http:\/\/$master_ip:11311/' .ros-root/setup_ros1_pepper.bash"
copy_script="sed -i.bak 's/^\(export\s*ROS_MASTER_URI\s*=\s*\).*$/\http:\/\/$master_ip:11311/' System/setup_ros1_pepper.bash"
Copy link
Owner

Choose a reason for hiding this comment

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

${HOST_INSTALL_ROOT}/setup_ros1_pepper.bash

Copy link
Owner

@esteve esteve Apr 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, I think it should be PEPPER_INSTALL_ROOT

install.sh Outdated
@@ -1,8 +1,10 @@
#! /bin/bash

HOST_INSTALL_ROOT="${BASE_ROOT:-${PWD}}/"System
Copy link
Owner

Choose a reason for hiding this comment

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

HOST_INSTALL_ROOT="${BASE_ROOT:-${PWD}}/System"

prepare_requirements_ros1.sh Outdated
@@ -4,7 +4,10 @@ set -euf -o pipefail

PYTHON2_VERSION=2.7.13

INSTALL_ROOT=.ros-root
HOST_INSTALL_ROOT="${BASE_ROOT:-${PWD}}/"System
Copy link
Owner

Choose a reason for hiding this comment

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

HOST_INSTALL_ROOT="${BASE_ROOT:-${PWD}}/System"

prepare_requirements_ros2.sh Outdated
@@ -1,11 +1,12 @@
#!/bin/bash

HOST_INSTALL_ROOT="${BASE_ROOT:-${PWD}}/"System
Copy link
Owner

Choose a reason for hiding this comment

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

HOST_INSTALL_ROOT="${BASE_ROOT:-${PWD}}/System"

build_ros1.sh Outdated
@@ -5,7 +5,8 @@ PYTHON2_PATCH_VERSION=13

PYTHON2_VERSION=${PYTHON2_MAJOR_VERSION}.${PYTHON2_MINOR_VERSION}.${PYTHON2_PATCH_VERSION}

INSTALL_ROOT=.ros-root
HOST_INSTALL_ROOT="${BASE_ROOT:-${PWD}}/"System
PEPPER_INSTALL_ROOT=System
Copy link
Owner

Choose a reason for hiding this comment

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

This changes where the ROS files are installed, it should be up to the user to decide, or at least keep the current directory (.ros-root)

fmrico
fmrico commented Apr 27, 2018
View reviewed changes
Copy link
Contributor Author

@fmrico fmrico left a comment

Choose a reason for hiding this comment

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

Changes made:

  1. BASE_ROOT changed to PEPPER_ROS_BASE_ROOT
  2. Updated Readme for removing .root-root
  3. Updated Readme for changing scp by rsync (much faster)

@esteve
Copy link
Owner

esteve commented Apr 29, 2018

@fmrico there's a few conflicts with master, could you rebase? Thanks!

I'm also thinking that using environment variables for setting the destination directory is just too fragile and it should be an argument that is passed to the scripts, what do you think?

esteve
esteve requested changes Apr 29, 2018
View reviewed changes
README.md Outdated
If you want to set the install directory (instead of the project root directory), set the `PEPPER_ROS_BASE_ROOT` environment variable:

```
export export PEPPER_ROS_BASE_ROOT=/home/mihome/pepper_root/ <-- Or wherever you want
Copy link
Owner

Choose a reason for hiding this comment

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

s/export export/export/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!!

@fmrico
Copy link
Contributor Author

fmrico commented May 22, 2018

@esteve Rebased!!!

fmrico added 18 commits May 25, 2018 10:24
@fmrico
Select install directory to build
3e86248
@fmrico
BASE_ROOT changed to PEPPER_ROS_BASE_ROOT
188cffc 
Readme updated

z#	modified:   setup_ros2_pepper.bash
@fmrico
Typo in readme.md
97903a1
@fmrico
Select install directory to build
6303afa
@fmrico
BASE_ROOT changed to PEPPER_ROS_BASE_ROOT
68b44d2 
Readme updated

z#	modified:   setup_ros2_pepper.bash
@fmrico
Typo in readme.md
636ecc2
@fmrico
Select install directory to build
780ed2a
@fmrico
BASE_ROOT changed to PEPPER_ROS_BASE_ROOT
1efd4af 
Readme updated

z#	modified:   setup_ros2_pepper.bash
@fmrico
Typo in readme.md
895cb85
@fmrico
Select install directory to build
f151aca
@fmrico
BASE_ROOT changed to PEPPER_ROS_BASE_ROOT
07d4474 
Readme updated

z#	modified:   setup_ros2_pepper.bash
@fmrico
Typo in readme.md
be084b5
@fmrico
Env var to BASE_ROOT
87a33a1
@fmrico
Fixed typo in install
7750c4d
@fmrico
Fixed var in install
2d0884e
@fmrico
Fixed var init in build_ros1
0c13bf6
@fmrico
Fixed var init in install
98e3c16
@fmrico
Fixed var init in prepare_reqs_ros1
b36c99d
@fmrico
Copy link
Contributor Author

fmrico commented May 25, 2018

@esteve , Comments are applied!

esteve
esteve requested changes Feb 5, 2019
View reviewed changes
README.md
If you want to set the install directory (instead of the project root directory), set the `PEPPER_ROS_BASE_ROOT` environment variable:

```
export PEPPER_ROS_BASE_ROOT=/home/mihome/pepper_root/ <-- Or wherever you want
Copy link
Owner

Choose a reason for hiding this comment

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

Replace /home/mihome with $HOME

README.md

We're going to copy these to the robot, assuming that your robot is connected to your network, type the following:

```
$ scp -r .ros-root nao@IP_ADDRESS_OF_YOUR_ROBOT:.ros-root
$ sync -avz System User nao@pepper.local:~/
Copy link
Owner

Choose a reason for hiding this comment

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

rsync instead of sync

README.md

We're going to copy these to the robot, assuming that your robot is connected to your network, type the following:

```
$ scp -r .ros-root nao@IP_ADDRESS_OF_YOUR_ROBOT:.ros-root
$ sync -avz System User nao@pepper.local:~/
Copy link
Owner

Choose a reason for hiding this comment

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

rsync

@fmrico fmrico mentioned this pull request Mar 9, 2019
Open
k-okada added a commit to k-okada/ros2_pepper that referenced this pull request Apr 27, 2022
@k-okada
adope @esteve comments from esteve#18
84c925b
@k-okada k-okada mentioned this pull request Apr 27, 2022
Open
k-okada added a commit to k-okada/ros2_pepper that referenced this pull request Apr 27, 2022
@k-okada
adopt @esteve comments from esteve#18
e2e73a4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@esteve esteve esteve requested 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
@fmrico @esteve