1494287 - mach bootstrap on macOS doesn't update cbindgen

Status: RESOLVED FIXED

(Firefox Build System :: Bootstrap Configuration, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Attachment: bootstrap output

I ran ./mach bootstrap for firefox desktop and it completed successfully. Then I tried to build and got an error about an out-of-date cbindgen. Output from bootstrap is attached.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

And this is what I got when I tried to build:

 0:31.78 checking for rustc... /Users/kats/.cargo/bin/rustc
 0:31.78 checking for cargo... /Users/kats/.cargo/bin/cargo
 0:32.32 checking rustc version... 1.29.1
 0:32.45 checking cargo version... 1.29.0
 0:35.72 checking for rustdoc... /Users/kats/.cargo/bin/rustdoc
 0:35.72 checking for cbindgen... /Users/kats/.cargo/bin/cbindgen
 0:35.76 checking cbindgen version...
 0:35.77 DEBUG: Executing: `/Users/kats/.cargo/bin/cbindgen --version`
 0:35.77 ERROR: cbindgen version 0.6.0 is too old. At least version 0.6.2 is required.
 0:35.77 Please update using 'cargo install cbindgen --force' or running
 0:35.77 './mach bootstrap', after removing the existing executable located at
 0:35.77 /Users/kats/.cargo/bin/cbindgen.
 0:35.82 *** Fix above errors and then restart with               "/Applications/Xcode.app/Contents/Developer/usr/bin/make -f client.mk build"
 0:35.83 make[1]: *** [configure] Error 1
 0:35.83 make: *** [/Users/kats/zspace/mozilla-wr/obj-host-prof/Makefile] Error 2
 0:35.85 302 compiler warnings present.
 0:36.31 ccache (direct) hit rate: 0.0%; (preprocessed) hit rate: 0.0%; miss rate: 100.0%

Comment 2

[Kartikaya Gupta (email:[e-mail])]

The "expected behaviour" here IMO is that bootstrap should check the cbindgen version installed and prompt to update it to a new version if needed. Or it should download a prebuilt cbindgen artifact from taskcluster.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Check existing cbindgen version and update if necessary.

Changed ensure_rust_package to receive minimum version, and check currently installed cbindgen version,
and update (force install newer version, because otherwise cargo install fails) if older version is found

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 9016139 [details] [diff] [review]
Check existing cbindgen version and update if necessary.

Review of attachment 9016139 [details] [diff] [review]:
-----------------------------------------------------------------

gps is travelling so I'll take this.

::: python/mozboot/mozboot/base.py
@@ +272,5 @@
>          cargo = self.which('cargo')
>          if not cargo:
>              cargo = os.path.join(cargo_bin, 'cargo')
> +
> +        list = subprocess.check_output([cargo, 'install', '--list', crate_name])

FYI, `cargo install --list` doesn't take any other arguments. The crate name is ignored here.

@@ +280,5 @@
> +            if line.startswith(target):
> +                version = line[len(target):-1]
> +                if LooseVersion(version) >= LooseVersion(min_version):
> +                    return
> +                found_version = version

Instead of using `found_version` you could put the `print('old version ...')` here, and after the loop have:
```
else:
    print('{name} not found ...')
```

(else blocks on for loops in Python are for when you fall off the end of the loop)

@@ +290,5 @@
> +            subprocess.check_call([cargo, 'install', '--force', crate_name])
> +        else:
> +            print('{name} not found, installing via cargo install.'.format(
> +                name=crate_name))
> +            subprocess.check_call([cargo, 'install', crate_name])

I would probably just move the `check_call` outside of the conditional entirely and always use `--force`, it won't hurt anything and will help avoid errors if someone changes this code in the future.

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9016139 [details] [diff] [review]
Check existing cbindgen version and update if necessary.

Review of attachment 9016139 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozboot/mozboot/osx.py
@@ +503,5 @@
>          return active_name.lower()
>  
>      def ensure_stylo_packages(self, state_dir, checkout_root):
>          # We installed clang via homebrew earlier.
> +        self.ensure_rust_package('cbindgen', '0.6.4')

There are other two callers of this function with cbindgen as an argument (openbsd.py and freebsd.py), this change probably breaks them.

Can we move the min version or this call somewhere common so that we don't need to duplicate the function three times, and mention the new place place to update in:

  https://dev.searchfox.org/mozilla-central/rev/50041c9be3154174195a75bf33de7339044482a1/taskcluster/scripts/misc/build-cbindgen.sh#4?

Comment 6

[(not currently active) Ted Mielczarek]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)
> There are other two callers of this function with cbindgen as an argument
> (openbsd.py and freebsd.py), this change probably breaks them.

I thought I had checked this, but I double-checked and both openbsd and freebsd use the system package manager to install cbindgen.

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Ouch, indeed, I was probably looking at an old checkout locally (this was changed very recently, in bug 1496708).

In any case please do leave a comment in build-cbindgen.sh, since otherwise it's easy to forget to update the version number :)

Comment 8

[Emilio Cobos Álvarez [:emilio]]

It'd be nice to not have the version number in three places, is there any way to do that ted? I guess we can avoid the duplication at least in configure / bootstrap...

Comment 9

[(not currently active) Ted Mielczarek]

We don't have a great story for that currently. We could define it as a constant in the mozboot package and import that into moz.configure.

Comment 10

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9de896fd363f935bba655ee6f8ef213bfbcd11cb
Bug 1494287 - Check existing cbindgen version and update if necessary. r=ted

Comment 11

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/965a5b3b29a0b3a2c0c89b0863992d9befbad616
Bug 1494287 - followup: Fix undefined variable in fallback path. r=me

Comment 12

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/9de896fd363f
https://hg.mozilla.org/mozilla-central/rev/965a5b3b29a0