Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11036032:
[MIPS] Script to build a trusted mipsel-linux-gnu cross toolchain from source. (Closed)
|
Created:
8 years, 2 months ago by petarj Modified:
8 years, 2 months ago CC:
native-client-reviews_googlegroups.com Base URL:
http://src.chromium.org/native_client/trunk/src/native_client/ Visibility:
Public. |
Description[MIPS] Script to build a trusted mipsel-linux-gnu cross toolchain from source.
This script is intended to build a mipsel-linux-gnu cross compilation toolchain
(from source) that runs on x86 linux and generates code for a little-endian,
hard-float, mips32 target.
The commit queue is not working for NaCl at the moment, so we are
committing with:
NOTRY=true
However, we have run a try job manually, and it passed fine
BUG= http://code.google.com/p/nativeclient/issues/detail?id=2275
TEST= run the script
Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=9978
Patch Set 1 #
Total comments: 12
Patch Set 2 : Update to the existing script. #
Total comments: 21
Patch Set 3 : Update per Robert's comments. #Patch Set 4 : Minor update - use GCC 4.7.2 (instead of 4.6.3). #Messages
Total messages: 15 (0 generated)
what is the relationship of this script with
tools/trusted_cross_toolchains/trusted-toolchain-creator.mipsel.squeeze.sh
could you use coding conventions similar to the other scripts
in that directory
in particular, use ${} for variables and use more functional abstraction to
eliminate repetition and make the code more readable.
On 4 October 2012 08:00, <robertm@google.com> wrote: > what is the relationship of this script with > tools/trusted_cross_toolchains/trusted-toolchain-creator.mipsel.squeeze.sh > It's a replacement that builds a trusted toolchain from source rather than downloading tarballs and .debs of prebuilt binaries. Mark -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To post to this group, send email to native-client-reviews@googlegroups.com. To unsubscribe from this group, send email to native-client-reviews+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
http://codereview.chromium.org/11036032/diff/1/tools/trusted_cross_toolchains... File tools/trusted_cross_toolchains/mips_tc_build.sh (right): http://codereview.chromium.org/11036032/diff/1/tools/trusted_cross_toolchains... tools/trusted_cross_toolchains/mips_tc_build.sh:16: set -eu Nit: add an empty line before this to separate it from the comment Can you do "set -eux"? The -x logs commands that are executed. This tends to make it easier to debug scripts when they are run on the buildbots. It also removes the need for some of the logging 'echo' commands below. http://codereview.chromium.org/11036032/diff/1/tools/trusted_cross_toolchains... tools/trusted_cross_toolchains/mips_tc_build.sh:21: bld_dir="$top_dir/obj" Can you make these less cryptic, please? i.e. "build_dir", "install_name", "install_dir" http://codereview.chromium.org/11036032/diff/1/tools/trusted_cross_toolchains... tools/trusted_cross_toolchains/mips_tc_build.sh:28: export PATH=$ins_dir/bin:/usr/sbin:/usr/bin:/sbin:/bin Please do export PATH=$ins_dir/bin:$PATH so that this coexists with whatever PATH the user has set. http://codereview.chromium.org/11036032/diff/1/tools/trusted_cross_toolchains... tools/trusted_cross_toolchains/mips_tc_build.sh:49: wget http://ftp.gnu.org/gnu/binutils/binutils-2.22.tar.bz2 Can you write wget http://ftp.gnu.org/gnu/binutils/binutils-2.22.tar.bz2 -O binutils-2.22.tar.bz2 Otherwise the script will fail if ~/.wgetrc contains "dirstruct = on" (which happens to be what I have :-) ). http://codereview.chromium.org/11036032/diff/1/tools/trusted_cross_toolchains... tools/trusted_cross_toolchains/mips_tc_build.sh:53: echo "binutils-2.22.tar.bz2 sha1sum failed, file deleted" Please indent blocks by 2 spaces http://codereview.chromium.org/11036032/diff/1/tools/trusted_cross_toolchains... tools/trusted_cross_toolchains/mips_tc_build.sh:55: exit This should be "exit 1" since this is an error http://codereview.chromium.org/11036032/diff/1/tools/trusted_cross_toolchains... tools/trusted_cross_toolchains/mips_tc_build.sh:60: if [ ! -f 'gcc-4.6.3.tar.bz2' ]; then There's a lot of duplication here. Can you factor the download into a function so that you can write: download_file gcc-4.6.3.tar.bz2 \ http://ftp.gnu.org/gnu/gcc/gcc-4.6.3/gcc-4.6.3.tar.bz2 \ ce317ca5c8185b58bc9300182b534608c578637f http://codereview.chromium.org/11036032/diff/1/tools/trusted_cross_toolchains... tools/trusted_cross_toolchains/mips_tc_build.sh:100: #untar all Please put a space after '#' in comments. And capitalise the first letter. http://codereview.chromium.org/11036032/diff/1/tools/trusted_cross_toolchains... tools/trusted_cross_toolchains/mips_tc_build.sh:112: #tar xzf $tar_dir/eglibc-2_14.tar.gz Please remove this commented-out line http://codereview.chromium.org/11036032/diff/1/tools/trusted_cross_toolchains... tools/trusted_cross_toolchains/mips_tc_build.sh:118: cd $src_dir/eglibc-2_14/libc/ && ln -s ../ports ports && cd - Can you replace "&&" with a newline, please? Same below. http://codereview.chromium.org/11036032/diff/1/tools/trusted_cross_toolchains... tools/trusted_cross_toolchains/mips_tc_build.sh:128: make --jobs=3 all-binutils all-gas all-ld || exit 1 "|| exit 1" isn't necessary since you have "set -e" at the start. Same below. http://codereview.chromium.org/11036032/diff/1/tools/trusted_cross_toolchains... tools/trusted_cross_toolchains/mips_tc_build.sh:130: mkdir -p $bld_dir/gcc/initial For readability, can you separate the build of each component with an empty line, please?
We changed the original script trusted-toolchain-creator.mipsel.squeeze.sh instead of making changes to the new one. Patch set #2 uploaded. Take a look.
On 5 October 2012 17:25, <petarj@mips.com> wrote: > We changed the original script trusted-toolchain-creator.** > mipsel.squeeze.sh <http://trusted-toolchain-creator.mipsel.squeeze.sh> > instead of making changes to the new one. > Patch set #2 uploaded. Take a look. > Hmm, I don't understand why you've done that. Your script from patch set 1 was nice in that it built everything from source. But the script in patch set 2 downloads Debian binary packages. I don't understand why you're keeping that aspect? You really only needed to make a few stylistic changes to your original script from patch set 1. Cheers, Mark -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To post to this group, send email to native-client-reviews@googlegroups.com. To unsubscribe from this group, send email to native-client-reviews+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
On 2012/10/06 00:40:38, Mark Seaborn wrote: > On 5 October 2012 17:25, <mailto:petarj@mips.com> wrote: > > > We changed the original script trusted-toolchain-creator.** > > mipsel.squeeze.sh <http://trusted-toolchain-creator.mipsel.squeeze.sh> > > instead of making changes to the new one. > > Patch set #2 uploaded. Take a look. > > > > Hmm, I don't understand why you've done that. > > Your script from patch set 1 was nice in that it built everything from > source. But the script in patch set 2 downloads Debian binary packages. I > don't understand why you're keeping that aspect? > > You really only needed to make a few stylistic changes to your original > script from patch set 1. @Mark Since the two scripts still need to be run one after another, we tried to integrate them into one. We someone understood that from your response to Robert in which you said this script is a replacement for the other one. The initial script (patch set 1) replaces only Mentor/CodeSourcery toolchain (binutils, gcc, eglibc and gdb). The rest of the libraries still need to be downloaded from Debian. Therefore, MIPS trusted toolchain creator script needs to be run afterwards. An alternative to downloading binaries is to build those dozen extra packages from sources in the script, but we would also need to build dependencies in that case, so the list would be much larger. Please advise. Thanks. Regards, Petar
On 2012/10/08 12:51:42, petarj wrote: Any further response on this one?
http://codereview.chromium.org/11036032/diff/6001/tools/trusted_cross_toolcha... File tools/trusted_cross_toolchains/trusted-toolchain-creator.mipsel.squeeze.sh (right): http://codereview.chromium.org/11036032/diff/6001/tools/trusted_cross_toolcha... tools/trusted_cross_toolchains/trusted-toolchain-creator.mipsel.squeeze.sh:85: DownloadOrCopy() { I think it would be clearer to have a separate function for checkouts. this way you do not have to overload the purpose of the second argument. Maybe change this in a subsequent CL http://codereview.chromium.org/11036032/diff/6001/tools/trusted_cross_toolcha... tools/trusted_cross_toolchains/trusted-toolchain-creator.mipsel.squeeze.sh:89: local checksum=${2} we usually do not use ${} for single char vars but it is ok http://codereview.chromium.org/11036032/diff/6001/tools/trusted_cross_toolcha... tools/trusted_cross_toolchains/trusted-toolchain-creator.mipsel.squeeze.sh:115: local calculated_checksum=`sha1sum ${filename} | cut -d ' ' -f 1` use $() instead of backticks http://codereview.chromium.org/11036032/diff/6001/tools/trusted_cross_toolcha... tools/trusted_cross_toolchains/trusted-toolchain-creator.mipsel.squeeze.sh:197: cd ${TMP}/eglibc-2_14/libc/ && ln -s ../ports ports && cd - we do not use long chains of && for the "ln" case just fix the args to ln for the sed case, why no define local variables, like local FILE_NAME="$PWD/tc-mips.c etc and the just say (cd XXX; sed YYY) # no need for cd - (cd XXX; a=b c=d http://codereview.chromium.org/11036032/diff/6001/tools/trusted_cross_toolcha... tools/trusted_cross_toolchains/trusted-toolchain-creator.mipsel.squeeze.sh:198: cd ${TMP}/binutils-2.22/gas/config \ add a comment for this http://codereview.chromium.org/11036032/diff/6001/tools/trusted_cross_toolcha... tools/trusted_cross_toolchains/trusted-toolchain-creator.mipsel.squeeze.sh:210: ${TMP}/binutils-2.22/configure --prefix=${INSTALL_ROOT} \ arg alignment http://codereview.chromium.org/11036032/diff/6001/tools/trusted_cross_toolcha... tools/trusted_cross_toolchains/trusted-toolchain-creator.mipsel.squeeze.sh:221: ${TMP}/gcc-4.6.3/configure --prefix=${INSTALL_ROOT} --disable-libssp \ one arg per line is easier to read (optional) http://codereview.chromium.org/11036032/diff/6001/tools/trusted_cross_toolcha... tools/trusted_cross_toolchains/trusted-toolchain-creator.mipsel.squeeze.sh:235: export PATH=${INSTALL_ROOT}/bin:$PATH it may be better to not export them put just prepend them before the configure command like so X=Y A=B configure .... http://codereview.chromium.org/11036032/diff/6001/tools/trusted_cross_toolcha... tools/trusted_cross_toolchains/trusted-toolchain-creator.mipsel.squeeze.sh:248: make ${MAKE_OPTS} install-headers install_root=${JAIL_MIPS32} install-bootstrap-headers=yes line length http://codereview.chromium.org/11036032/diff/6001/tools/trusted_cross_toolcha... tools/trusted_cross_toolchains/trusted-toolchain-creator.mipsel.squeeze.sh:256: unset BUILD_CC AR RANLIB CC CXX comment? (may not be needed anymore if you follow the suggestion above) http://codereview.chromium.org/11036032/diff/6001/tools/trusted_cross_toolcha... tools/trusted_cross_toolchains/trusted-toolchain-creator.mipsel.squeeze.sh:270: export BUILD_CC=gcc see above
Patch set #3 has been uploaded. https://chromiumcodereview.appspot.com/11036032/diff/6001/tools/trusted_cross... File tools/trusted_cross_toolchains/trusted-toolchain-creator.mipsel.squeeze.sh (right): https://chromiumcodereview.appspot.com/11036032/diff/6001/tools/trusted_cross... tools/trusted_cross_toolchains/trusted-toolchain-creator.mipsel.squeeze.sh:85: DownloadOrCopy() { On 2012/10/10 15:38:27, robertm wrote: > I think it would be clearer to have a separate function for checkouts. this way > you do not have to overload the purpose > of the second argument. > Maybe change this in a subsequent CL Done. https://chromiumcodereview.appspot.com/11036032/diff/6001/tools/trusted_cross... tools/trusted_cross_toolchains/trusted-toolchain-creator.mipsel.squeeze.sh:89: local checksum=${2} On 2012/10/10 15:38:27, robertm wrote: > we usually do not use ${} for single char vars but it is ok Done. https://chromiumcodereview.appspot.com/11036032/diff/6001/tools/trusted_cross... tools/trusted_cross_toolchains/trusted-toolchain-creator.mipsel.squeeze.sh:115: local calculated_checksum=`sha1sum ${filename} | cut -d ' ' -f 1` On 2012/10/10 15:38:27, robertm wrote: > use $() instead of backticks Done. https://chromiumcodereview.appspot.com/11036032/diff/6001/tools/trusted_cross... tools/trusted_cross_toolchains/trusted-toolchain-creator.mipsel.squeeze.sh:197: cd ${TMP}/eglibc-2_14/libc/ && ln -s ../ports ports && cd - On 2012/10/10 15:38:27, robertm wrote: > we do not use long chains of && > > for the "ln" case just fix the args to ln > > for the sed case, why no define local variables, like > local FILE_NAME="$PWD/tc-mips.c > etc > and the just say > (cd XXX; sed YYY) # no need for cd - > > (cd XXX; a=b c=d Done. https://chromiumcodereview.appspot.com/11036032/diff/6001/tools/trusted_cross... tools/trusted_cross_toolchains/trusted-toolchain-creator.mipsel.squeeze.sh:198: cd ${TMP}/binutils-2.22/gas/config \ On 2012/10/10 15:38:27, robertm wrote: > add a comment for this Done. https://chromiumcodereview.appspot.com/11036032/diff/6001/tools/trusted_cross... tools/trusted_cross_toolchains/trusted-toolchain-creator.mipsel.squeeze.sh:210: ${TMP}/binutils-2.22/configure --prefix=${INSTALL_ROOT} \ On 2012/10/10 15:38:27, robertm wrote: > arg alignment Done. https://chromiumcodereview.appspot.com/11036032/diff/6001/tools/trusted_cross... tools/trusted_cross_toolchains/trusted-toolchain-creator.mipsel.squeeze.sh:221: ${TMP}/gcc-4.6.3/configure --prefix=${INSTALL_ROOT} --disable-libssp \ On 2012/10/10 15:38:27, robertm wrote: > one arg per line is easier to read (optional) Done. https://chromiumcodereview.appspot.com/11036032/diff/6001/tools/trusted_cross... tools/trusted_cross_toolchains/trusted-toolchain-creator.mipsel.squeeze.sh:235: export PATH=${INSTALL_ROOT}/bin:$PATH On 2012/10/10 15:38:27, robertm wrote: > it may be better to not export them put just > prepend them before the configure command like so > X=Y A=B configure .... Done. https://chromiumcodereview.appspot.com/11036032/diff/6001/tools/trusted_cross... tools/trusted_cross_toolchains/trusted-toolchain-creator.mipsel.squeeze.sh:248: make ${MAKE_OPTS} install-headers install_root=${JAIL_MIPS32} install-bootstrap-headers=yes On 2012/10/10 15:38:27, robertm wrote: > line length Done. https://chromiumcodereview.appspot.com/11036032/diff/6001/tools/trusted_cross... tools/trusted_cross_toolchains/trusted-toolchain-creator.mipsel.squeeze.sh:270: export BUILD_CC=gcc On 2012/10/10 15:38:27, robertm wrote: > see above Done.
LGTM
CQ is trying da patch. Follow status at https://nativeclient-status.appspot.com/cq/petarj@mips.com/11036032/12001
The commit queue went berserk retrying too often for a seemingly flaky test. Builder is nacl-arm_opt_panda, revision is 9976, job name was 11036032-12001 (previous was lost) (previous was lost) (previous was lost) (previous was lost).
CQ is trying da patch. Follow status at https://nativeclient-status.appspot.com/cq/petarj@mips.com/11036032/17001
Change committed as 9978 |
