Remove all default architecture support in cranelift-codegen to allow dependencies fine-grained control

[mbestavros]

This is a small patch for an issue originally brought up over on the Wasmtime repository: bytecodealliance/wasmtime#189

In a nutshell, cranelift-codegen had enabled all architecture support in its default feature, and, for one reason or another, it's very difficult to disable the default feature from within Wasmtime. This makes it hard for projects that want to pick and choose which architectures they want Wasmtime to build for.

As a result, the solution pitched was to remove all architecture support from the default feature and allow dependencies to enable the exact architecture support they wanted. That's what this patch hopes to achieve.

For ease of use, there is a new all-arch feature included that enables all architecture support -- just as cranelift-codegen behaved before.

One possible area for improvement: this code does not currently enforce that at least one of the architectures must be enabled. I briefly looked into using required-features, but it seems like that wouldn't apply here. I couldn't find other solutions that might work, but I'm open to suggestions if I missed something obvious.

Tagging @npmccallum, @alexcrichton, @sunfishcode, @steveej for review.

[bjorn3]

You can check that at least one of the archs is enabled in build.rs

[mbestavros]

Interesting: having looked at build.rs for cranelift-codegen, it seems like logic already exists for handling the case of no architectures being enabled; it'll try and match the native target.

If build.rs were to enforce an architecture being enabled, this code wouldn't really be needed, unless I'm missing something. What would the preferred approach be? @alexcrichton or @sunfishcode, curious to hear your thoughts, if any, on this.

[mbestavros]

Having taken another look at build.rs, it seems like it does panic if it doesn't have any ISAs to use -- though it seems like the only time that will happen is if cargo was manually configured to not use any architectures, and it can't identify the native target correctly.

I think the conversation comes back to whether we want build.rs to guess the native target, or to simply panic if ISA targets aren't manually specified.

[sunfishcode]

I think either way is basically ok. I kind of like a panic, but compiling for the native host is going to be a common use case, so it's not a bad thing if it's convenient to do that.

[mbestavros]

@sunfishcode, amended that comment.

If you see no issues with figuring out architecture support in build.rs, I don't see a need to remove what's there.

It may be worth mentioning that code has probably never been run, since the existing default configuration prevented an empty architecture set (and the consequent architecture derivation) in all cases. Not sure if that's a concern or not, but maybe worth noting.

[mbestavros]

@sunfishcode, bump -- any other thoughts on this?

[bnjbvr]

Hi, Spidermonkey hacker here (I think Dan is away for a few days). We've been using default-features=false in projects that were depending on Cranelift, and then re-enabling features manually in the embedder project (with `default = ['cranelift-codegen/std', 'cranelift-codegen/x86']). So this code has been running and is being tested :).

Have you tried this? If that's the thing that didn't work, have you tried making sure that features were not getting re-enabled by other cranelift crates?

Or are you saying that the sane default would be to have no arch at all, and that arch support should be opt-in? I think I like this better, if that's the case, and agree with this PR.

[mbestavros]

I had a heck of a time trying to get default-features=false to work, and I didn't end up having any success. @alexcrichton addressed it in the original bug around this here: bytecodealliance/wasmtime#189 Maybe it's a project-specific issue?

Your last paragraph is more or less the gist of what Dan and my team were discussing; given the small number of projects (and the scope of those projects) likely to depend on Cranelift, it's not too much of an ask for them to specify exactly which architectures they want to support. That's what I was implementing here.

[bnjbvr]

Great, then 👍.

[mbestavros]

@sunfishcode -- is there anything else outstanding blocking this?

[bnjbvr]

@mbestavros Note this breaks CI testing, can you have a look, please?

[mbestavros]

@bnjbvr Looks like like the Travis tests don't account for the new feature -- the failures all throw an error around "no ARM support". This makes sense, given there is now no architecture support at all by default with this PR.

The quick fix would be to invoke the all-arch feature included with this PR when running tests on Travis. For now, that's probably what makes the most sense -- unless people think the tests should be amended further to account for this change.

[bnjbvr]

Could the top-level Cargo.toml (the one that controls the binary programs) enable all features by default? That'd be useful for using the clif-util too, and would fix CI.

[mbestavros]

@bnjbvr That's the current behavior; this PR is specifically designed to change it. See the original PR comment for details.

[bnjbvr]

@mbestavros I am not entirely sure we're talking about the same thing: this commit updates cranelift-codegen/Cargo.toml while my previous comment was suggesting we could update Cargo.toml. I don't think the top-level Cargo.toml exposes any crate, since embedders should at most use cranelift-umbrella if they want everything. Am I missing something here?

[mbestavros]

@bnjbvr Ah, I see what you mean now -- apologies.

That makes sense. I'll try making that change now.

[bnjbvr]

Nice, thanks. Testing actually passed on MacOS, but failed because of caching issues (initially on Linux, eventually on MacOS), so I've restarted the build.

[bnjbvr]

Sweet, thanks!