1312916 - Allow building host Rust crates in the build system

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

I'd like to use some Rust code in dump_syms, which is a host binary. We don't currently support building host Rust crates. Talking with froydnj about it on IRC we determined it shouldn't be terribly hard, we just need to pass the right --target to cargo and do the moz.build plumbing.

Comment 1

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1312916 - Add support for host rust libraries in moz.build.

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

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8819448 [details]
Bug 1312916 - Add support for host rust libraries in moz.build.

https://reviewboard.mozilla.org/r/99210/#review100278

This patch is basically what I expected, so that's good!  Just a few small things to fix, I think, but otherwise pretty solid.  Interested to hear feedback on the `BaseX` comments for `HostRustLibrary` and the suggested test.

::: config/rules.mk:967
(Diff revision 1)
>  endif # RUST_LIBRARY_FILE
>  
> +ifdef HOST_RUST_LIBRARY_FILE
> +force-cargo-host-library-build:
> +	$(REPORT_BUILD)
> +	$(CARGO_BUILD) --lib $(cargo_host_flag) $(rust_features_flag)

Wouldn't we want separate feature lists for target Rust libraries vs. host ones?

::: python/mozbuild/mozbuild/frontend/data.py:638
(Diff revision 1)
>  class HostLibrary(HostMixin, BaseLibrary):
>      """Context derived container object for a host library"""
>      KIND = 'host'
>  
>  
> +class HostRustLibrary(HostMixin, RustLibrary):

I think our usual pattern for this is a `BaseX`, from which we have host and target versions, rather than having the host version inherit directly from the target version.

::: python/mozbuild/mozbuild/test/backend/test_recursivemake.py:752
(Diff revision 1)
>              'CARGO_FILE := $(srcdir)/Cargo.toml',
>          ]
>  
>          self.assertEqual(lines, expected)
>  
> +    def test_rust_host_library(self):

May want a host-with-features test here.

Comment 3

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8819448 [details]
Bug 1312916 - Add support for host rust libraries in moz.build.

https://reviewboard.mozilla.org/r/99210/#review100278

> Wouldn't we want separate feature lists for target Rust libraries vs. host ones?

I guess it's not crazy to have a host and target rust library in the same directory, sure.

> May want a host-with-features test here.

Sure.

Comment 4

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8819448 [details]
Bug 1312916 - Add support for host rust libraries in moz.build.

https://reviewboard.mozilla.org/r/99210/#review100278

> I think our usual pattern for this is a `BaseX`, from which we have host and target versions, rather than having the host version inherit directly from the target version.

So I agree it doesn't make a lot of sense to have a Host thing inherit from a Target thing, but the alternative gets into a bit of a multiple inheritance mess. Anyway, I'll upload it and we can decide which is worse.

Comment 5

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8819448 [details]
Bug 1312916 - Add support for host rust libraries in moz.build.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99210/diff/1-2/

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8819448 [details]
Bug 1312916 - Add support for host rust libraries in moz.build.

https://reviewboard.mozilla.org/r/99210/#review106366

Thanks for revising this.

::: build/templates.mozbuild:140
(Diff revision 2)
> +    HostLibrary(name)
> +
> +    IS_RUST_LIBRARY = True
> +
> +    if features:
> +        RUST_LIBRARY_FEATURES = features

My initial impulse was to say that this should be `HOST_RUST_LIBRARY_FEATURES`.  But I guess it'd be difficult to have a `HostRustLibrary` and a `RustLibrary` defined in the same moz.build file with our current setup, wouldn't it?  Or, at least, nobody's particularly concerned with such a thing at the moment.  (Maybe I was silly for asking for things to be duplicated in my previous review.)

Please do modify the documentation for `RUST_LIBRARY_FEATURES` to note that `RUST_LIBRARY_FEATURES` can be used for host or target features.

::: python/mozbuild/mozbuild/backend/recursivemake.py:1209
(Diff revision 2)
> -        backend_file.write_once('RUST_LIBRARY_FILE := %s\n' % libdef.import_name)
> +        lib_var = 'RUST_LIBRARY_FILE'
> +        feature_var = 'RUST_LIBRARY_FEATURES'
> +        if isinstance(libdef, HostRustLibrary):
> +            lib_var = 'HOST_RUST_LIBRARY_FILE'
> +            feature_var = 'HOST_RUST_LIBRARY_FEATURES'

Perhaps we should just move these to be static members on the `RustLibrary` and `HostRustLibrary` classes?

Comment 7

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8819448 [details]
Bug 1312916 - Add support for host rust libraries in moz.build.

https://reviewboard.mozilla.org/r/99210/#review106366

> My initial impulse was to say that this should be `HOST_RUST_LIBRARY_FEATURES`.  But I guess it'd be difficult to have a `HostRustLibrary` and a `RustLibrary` defined in the same moz.build file with our current setup, wouldn't it?  Or, at least, nobody's particularly concerned with such a thing at the moment.  (Maybe I was silly for asking for things to be duplicated in my previous review.)
> 
> Please do modify the documentation for `RUST_LIBRARY_FEATURES` to note that `RUST_LIBRARY_FEATURES` can be used for host or target features.

I thought about this, and the separate variable for host features seems fine. I'm inclined to just fix the typo at this point, thank you for noticing it!

Comment 8

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8819448 [details]
Bug 1312916 - Add support for host rust libraries in moz.build.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99210/diff/2-3/

Comment 9

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8819448 [details]
Bug 1312916 - Add support for host rust libraries in moz.build.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99210/diff/3-4/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e0fb919e31cf
Add support for host rust libraries in moz.build. r=froydnj

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e0fb919e31cf