1399882 - Wrap cargo invocation in a Python script

Status: RESOLVED DUPLICATE of bug 1522614

(Firefox Build System :: General, enhancement)

Description

[(not currently active) Ted Mielczarek]

Currently we have a bunch of make and shell logic to invoke cargo with the right set of arguments and environment variables:
https://dxr.mozilla.org/mozilla-central/rev/f9a5e9ed62103c84e4cde915f4d08f1ce71be83e/config/rules.mk#841-1057

The syntax leaves something to be desired, given the available tooling. Additionally, I can't make this work in WSL because environment variables are not persisted to native Windows processes. Given that, I'd like to move this all into a Python script so we can express all these conditionals in Python instead of make+shell, and also give me a more useful place to inject some WSL-specific logic.

Comment 1

[(not currently active) Ted Mielczarek]

Attachment: bug 1399882 - Wrap cargo invocation in a Python script.

Currently rules.mk contains a lot of logic to assemble the commandline and
environment for running cargo. This is messy because GNU make does not
give you very good tools to work with (and shell isn't much better).

This patch adds a `cargo` script to mozbuild.action that wraps the cargo
invocation and moves most of the logic into the script.

Review commit: https://reviewboard.mozilla.org/r/180350/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/180350/

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8908718 [details]
bug 1399882 - Wrap cargo invocation in a Python script.

https://reviewboard.mozilla.org/r/180350/#review185522

Can't see much else to do with this.  This is much nicer than the Makefile-based variant, thank you!

::: python/mozbuild/mozbuild/action/cargo.py:23
(Diff revision 1)
> +    parser.add_argument('--lib', action='store_true',
> +                        help='Build a library')
> +    parser.add_argument('--host-lib', action='store_true',
> +                        help='Build a host library')
> +    parser.add_argument('--bin', nargs='*',
> +                        help='Build a binary')
> +    parser.add_argument('--host-bin', nargs='*',
> +                        help='Build a host binary')

WDYT about trying to ensure that these options are all mutually exclusive?

::: python/mozbuild/mozbuild/action/cargo.py:33
(Diff revision 1)
> +    parser.add_argument('command', help='cargo subcommand to invoke')
> +    parser.add_argument('manifest_path',
> +                        help='Cargo manifest to build')
> +    parser.add_argument('target_dir',
> +                        help='Directory in which to build')

Probably doesn't matter greatly here, but do you want to add `metavar=...` arguments for each of these?

::: python/mozbuild/mozbuild/action/cargo.py:130
(Diff revision 1)
> +        # Cargo needs the same linker flags as the C/C++ compiler,
> +        # but not the final libraries. Filter those out because they
> +        # cause problems on macOS 10.7; see bug 1365993 for details.

Can you file a followup to see if we can get rid of this now that we don't build on 10.7?  I guess we may want to keep the custom linker regardless for cross builds, but we could get rid of the Mac-specific hacks.

::: python/mozbuild/mozbuild/action/cargo.py:155
(Diff revision 1)
> +    )
> +    for k in env.keys():
> +        if isinstance(env[k], unicode):
> +            env[k] = env[k].encode(sys.getfilesystemencoding() or 'mbcs')
> +    keys = set(env.keys()) - set(os.environ.keys())
> +    print('%s %s' % (' '.join('%s=%s' % (k, quote(env[k])) for k in keys), ' '.join(quote(c) for c in cmd)))

This is just debugging material?  Or do we think this is useful to have in (maybe under `BUILD_VERBOSE_LOG`) so people can see what's going on?

Comment 3

[(not currently active) Ted Mielczarek]

I think glandium fixed this in a more sensible way in bug 1522614.