Clang update script: download pre-built modern gcc and CMake if necessary

Clang update script: download pre-built modern gcc and CMake if necessary

This allows Precise builders using LLVM_FORCE_HEAD_REVISION to build
LLVM and Clang even though their system gcc and cmake are too old.

BUG=452726
NOTRY=true

Committed: https://crrev.com/8980eb7a634ec8de9cf1b7899f47939eb5cbd291
Cr-Commit-Position: refs/heads/master@{#319130}

[hans, 2015-03-04 20:45:30]

hans@chromium.org changed reviewers:
+ thakis@chromium.org

[hans, 2015-03-04 20:45:30]

Please take a look.

[Nico, 2015-03-04 20:51:19]

lgtm, that's a lot shorter than i feared

https://codereview.chromium.org/980633003/diff/1/tools/clang/scripts/update.sh
File tools/clang/scripts/update.sh (right):

https://codereview.chromium.org/980633003/diff/1/tools/clang/scripts/update.s...
tools/clang/scripts/update.sh:174: fi
nit: only do this if gcc_toolchain isn't set (?)

[hans, 2015-03-04 21:00:13]

maybe take another look?

https://codereview.chromium.org/980633003/diff/1/tools/clang/scripts/update.sh
File tools/clang/scripts/update.sh (right):

https://codereview.chromium.org/980633003/diff/1/tools/clang/scripts/update.s...
tools/clang/scripts/update.sh:174: fi
On 2015/03/04 20:51:19, Nico wrote:
> nit: only do this if gcc_toolchain isn't set (?)

Oh, good point. Actually, should also check for Linux.

[Nico, 2015-03-04 21:03:01]

still lgtm

https://codereview.chromium.org/980633003/diff/20001/tools/clang/scripts/upda...
File tools/clang/scripts/update.sh (right):

https://codereview.chromium.org/980633003/diff/20001/tools/clang/scripts/upda...
tools/clang/scripts/update.sh:168: if [[ "${OS}" == "Linux" ]] && [[ -z
${gcc_toolchain:-''} ]]; then
Isn't it usually [[ --n "$gcc_toolchain"]]?

Also, I think you can [[ "${OS}" == "Linux" -a -n "$gcc_toolchain" ]]

[hans, 2015-03-04 21:15:57]

On 2015/03/04 21:03:01, Nico wrote:
> still lgtm
> 
>
https://codereview.chromium.org/980633003/diff/20001/tools/clang/scripts/upda...
> File tools/clang/scripts/update.sh (right):
> 
>
https://codereview.chromium.org/980633003/diff/20001/tools/clang/scripts/upda...
> tools/clang/scripts/update.sh:168: if [[ "${OS}" == "Linux" ]] && [[ -z
> ${gcc_toolchain:-''} ]]; then
> Isn't it usually [[ --n "$gcc_toolchain"]]?

(I assume you mean -n?)

I think we need -z ("is zero-length"), but the :-'' thing is unnecessary since
the variable should always be bound at this point.

> Also, I think you can [[ "${OS}" == "Linux" -a -n "$gcc_toolchain" ]]

We seem to be using && throughout the file, so I'll keep that.

[hans, 2015-03-04 21:16:35]

The CQ bit was checked by hans@chromium.org

[hans, 2015-03-04 21:16:35]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/980633003/#ps40001 (title:
"Simplify")

[commit-bot: I haz the power, 2015-03-04 21:17:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/980633003/40001

[hans, 2015-03-04 21:22:51]

The CQ bit was unchecked by hans@chromium.org

[hans, 2015-03-04 21:23:09]

The CQ bit was checked by hans@chromium.org

[commit-bot: I haz the power, 2015-03-04 21:23:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/980633003/40001

[commit-bot: I haz the power, 2015-03-04 21:24:20]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-04 21:25:01]

Patchset 3 (id:??) landed as
https://crrev.com/8980eb7a634ec8de9cf1b7899f47939eb5cbd291
Cr-Commit-Position: refs/heads/master@{#319130}

[Nico, 2015-03-05 03:55:18]

https://codereview.chromium.org/980633003/diff/40001/tools/clang/scripts/upda...
File tools/clang/scripts/update.sh (right):

https://codereview.chromium.org/980633003/diff/40001/tools/clang/scripts/upda...
tools/clang/scripts/update.sh:171: if [[ ! -e "${LLVM_BUILD_TOOLS_DIR}/gcc482"
]]; then
This if seems to not work for some reason, looks like the bots download stuff on
every run.

[Nico, 2015-03-05 03:58:39]

https://codereview.chromium.org/980633003/diff/40001/tools/clang/scripts/upda...
File tools/clang/scripts/update.sh (right):

https://codereview.chromium.org/980633003/diff/40001/tools/clang/scripts/upda...
tools/clang/scripts/update.sh:194: export
PATH="${LLVM_BUILD_TOOLS_DIR}/cmake310/bin:${PATH}"
…and this should probably run before the `cmake --version` line. If the
directory doesn't exist the if will then be entered if cmake is too old, and
won't be entered if cmake was already downloaded before

[hans, 2015-03-05 04:20:38]

On 2015/03/05 03:55:18, Nico wrote:
>
https://codereview.chromium.org/980633003/diff/40001/tools/clang/scripts/upda...
> File tools/clang/scripts/update.sh (right):
> 
>
https://codereview.chromium.org/980633003/diff/40001/tools/clang/scripts/upda...
> tools/clang/scripts/update.sh:171: if [[ ! -e "${LLVM_BUILD_TOOLS_DIR}/gcc482"
> ]]; then
> This if seems to not work for some reason, looks like the bots download stuff
on
> every run.

It's the bot_update step:

> ===Running git clean -dff (retry #1)===
> Removing third_party/llvm-build-tools/
> ===Succeeded in 0.0 mins===

We could put things under third_party/llvm-build, but the script is also looking
at that dir to determine if llvm has been built before. Maybe that's not a
problem though.

It is kind of nice to always start from a known-good state though. Do you think
downloading on each run is a big problem? We could drop the -v flag from tar to
make it less noisy.

[hans, 2015-03-05 04:23:41]

On 2015/03/05 03:58:39, Nico wrote:
>
https://codereview.chromium.org/980633003/diff/40001/tools/clang/scripts/upda...
> File tools/clang/scripts/update.sh (right):
> 
>
https://codereview.chromium.org/980633003/diff/40001/tools/clang/scripts/upda...
> tools/clang/scripts/update.sh:194: export
> PATH="${LLVM_BUILD_TOOLS_DIR}/cmake310/bin:${PATH}"
> …and this should probably run before the `cmake --version` line. If the
> directory doesn't exist the if will then be entered if cmake is too old, and
> won't be entered if cmake was already downloaded before

It feels a bit ugly to add a non-existing dir to PATH if the system cmake is new
enough. Someone looking at the path later might get confused about what we're
using.

The code already won't download if the dir exists. Isn't that good enough? The
dir also has a version number in it, so if we decide to move to a newer one we'd
use a different dir name and not get stuck on an old one.

[Nico, 2015-03-05 04:26:28]

You're right, I guess downloading this every time isn't really a problem.
(Makes it a bit harder to find the LLVM revision when looking for
regression ranges, but I'll just have to learn to cmd-f "updated to
revision" or something)

On Wed, Mar 4, 2015 at 8:23 PM, <hans@chromium.org> wrote:

> On 2015/03/05 03:58:39, Nico wrote:
>
> https://codereview.chromium.org/980633003/diff/40001/
> tools/clang/scripts/update.sh
>
>> File tools/clang/scripts/update.sh (right):
>>
>
>
> https://codereview.chromium.org/980633003/diff/40001/
> tools/clang/scripts/update.sh#newcode194
>
>> tools/clang/scripts/update.sh:194: export
>> PATH="${LLVM_BUILD_TOOLS_DIR}/cmake310/bin:${PATH}"
>> …and this should probably run before the `cmake --version` line. If the
>> directory doesn't exist the if will then be entered if cmake is too old,
>> and
>> won't be entered if cmake was already downloaded before
>>
>
> It feels a bit ugly to add a non-existing dir to PATH if the system cmake
> is new
> enough. Someone looking at the path later might get confused about what
> we're
> using.
>
> The code already won't download if the dir exists. Isn't that good enough?
> The
> dir also has a version number in it, so if we decide to move to a newer
> one we'd
> use a different dir name and not get stuck on an old one.
>
> https://codereview.chromium.org/980633003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.