Closed Bug 1399882 Opened 3 years ago Closed 2 years ago

Wrap cargo invocation in a Python script

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox57 wontfix)

RESOLVED DUPLICATE of bug 1522614
Tracking Status
firefox57 --- wontfix

People

(Reporter: ted, Assigned: ted)

References

Details

Attachments

(1 file)

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 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?
Attachment #8908718 - Flags: review?(nfroyd) → review+
Product: Core → Firefox Build System

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

Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1522614
You need to log in before you can comment on or make changes to this bug.