[TRAFODION-2802] Prepare the build environment with one command #1353
Conversation
|
Can one of the admins verify this patch? |
3 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Please do not open new pull-requests, when an existing pull-request exists for the same change/jira. It is better to just push new commits onto the same branch so that we can see previous commits & changes. If it is absolutely necessary to do so, then it is good idea to put comment in the older one and close it. |
|
jenkins, ok to test |
|
Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/2271/ |
|
Test Passed. https://jenkins.esgyn.com/job/Check-PR-master/2271/ |
|
The old pull-request is already closed with related comment, thanks. |
install/traf_checkset_env.sh
Outdated
| exit 1 | ||
| fi | ||
|
|
||
| # check the based command |
There was a problem hiding this comment.
Maybe better to say "check some basic commands"
install/traf_checkset_env.sh
Outdated
| lsb_release) | ||
| (${MY_YUM} install redhat-lsb) >>${LOGFILE}2>&1 | ||
| if [ "x$?" = "x0" ]; then | ||
| echo "ERROR: yum repo server has a error when run command [${MY_YUM} install redhat-lsb]." |
There was a problem hiding this comment.
Typo: "has an error when running"
install/traf_checkset_env.sh
Outdated
| fi | ||
| ;; | ||
| *) | ||
| echo "ERROR: command [${basecmd}] not exist. Make sure you have installed it, and have added it to the command path." |
There was a problem hiding this comment.
Typo: "not exist" -> "does not exist"
install/traf_checkset_env.sh
Outdated
| exit 1 | ||
| esac | ||
| fi | ||
| echo "INFO: command ${basecmd} exist" |
| echo "INFO: os distributor id is [${osdistributor}]" | ||
| ;; | ||
| *) | ||
| echo "ERROR: The OS distribution is [${osdistributor}], but Trafodion only supports RedHat and CentOS presently." |
There was a problem hiding this comment.
As far as I know we have also tried on the Oracle and Amazon distributions, which are similar to RedHat, and they may work as well (possibly with some small modifications). Many of the other Linux distros don't have yum, so they would have given an error above. Maybe you want to allow the user to proceed, at their own risk?
There was a problem hiding this comment.
I don‘t have the environment as you say, if the user has this environment, user can installed by modifying the environment variable MY_YUM
install/traf_checkset_env.sh
Outdated
| # must configure the yum repo right before execute this script. | ||
| # run this script with normal user, while must has sudo permission. | ||
|
|
||
| #default path |
There was a problem hiding this comment.
This command needs a help text that should mention the following:
- Who should use this script (people who want to set up a single node
development system on a machine where they have sudo privileges) - What does the script do (e.g. yum install, build/setup tools).
- A link to the Contributor guide wiki page that describes these steps
(https://cwiki.apache.org/confluence/display/TRAFODION/Create+Build+Environment) - What environment variables must be set before calling this script
- What optional environment variables can be set
- What directories are created by the script, and how they can be customized
Once this PR is merged, we can also update the wiki page and mention this command.
install/traf_checkset_env.sh
Outdated
| MY_YUM=${MY_YUM-"${MY_SUDO} yum -y"} | ||
|
|
||
| # for setup tools | ||
| MY_LOCAL_SW_DIST=${MY_LOCAL_SW_DIST-${HOME}/local_software_tools} |
There was a problem hiding this comment.
It's pretty common that the root directory of a server is small (at least on the OpenStack and Amazon VMs I have seen). Maybe add yet another variable like this one:
MY_SW_HOME=${MY_SW_HOME-${HOME}}
Then use $MY_SW_HOME instead of $HOME below.
install/traf_checkset_env.sh
Outdated
| echo "INFO: install dependent library finish" | ||
|
|
||
| # remove pdsh and qt-dev | ||
| echo "INFO: remove pdsh and qt-dev trafodion scripts get confused." |
There was a problem hiding this comment.
Maybe say "INFO: remove pdsh and qt-dev commands to avoid problems with trafodion scripts"
install/traf_checkset_env.sh
Outdated
|
|
||
| javahome=`grep JAVA_HOME ~/.bashrc | wc -l` | ||
| if [ "x${javahome}" = "x0" ]; then | ||
| echo -en "export JAVA_HOME=${javapath}\n" >> $HOME/.bashrc |
There was a problem hiding this comment.
When writing into the users .bashrc, I would recommend adding a comment that shows what tool wrote this. Something like:
# Lines added by traf_checkset_env.sh
exprt JAVA_HOME=...
install/traf_checkset_env.sh
Outdated
| if [ "x${javahome}" = "x0" ]; then | ||
| echo -en "export JAVA_HOME=${javapath}\n" >> $HOME/.bashrc | ||
| echo -en "export PATH=\$PATH:\$JAVA_HOME/bin\n" >> $HOME/.bashrc | ||
| fi |
There was a problem hiding this comment.
It would probably be good to add a test here that runs
$JAVA_HOME/bin/java -version
and checks that we get an acceptable version.
|
New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/2281/ |
|
Test Passed. https://jenkins.esgyn.com/job/Check-PR-master/2281/ |
|
Can one of the admins verify this patch? |
2 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
New Check Test Started: https://jenkins.esgyn.com/job/Check-PR-master/2283/ |
|
Can one of the admins verify this patch? |
|
Test Passed. https://jenkins.esgyn.com/job/Check-PR-master/2283/ |
|
Is this ready to merge now? @zellerh, what do you think? |
|
+1 |
|
Thanks for this contribution. Sorry it's taken so long to merge. |
No description provided.