Don't set definition flags for MSVC ARM targets

[tleasor]

This is a simple fix for #516

[alexcrichton]

Thanks for this! I'm wondering though if it's perhaps best to panic or error in these cases? It seems sort of bad to silently ignore user input?

I'm not really sure of the best way to handle this unfortunately if it's a feature that's not supported by just one assembler...

[tleasor]

That's a good point, I guess it ultimately comes down to where one thinks this quirk should be taken into account (i.e. at the cc-rs level or at the level of a project that depends on cc-rs).

On the one hand someone that's writing assembly for this target presumably knows that they can't use these -D flags with the relevant assemblers so would avoid using .define() for these targets in their Rust code. On the other hand, they wouldn't be able to use text macros in MSVC ARM assembly anyway so might assume that cc-rs should "just work" and compile without any issues (and ignore any -D flags in doing so).

I think both viewpoints are valid, and would be happy to go the route of a panic or error in this (or a new) PR if you think that silently ignoring these flags isn't the right way to go.

I'm not really sure of the best way to handle this unfortunately if it's a feature that's not supported by just one assembler...

I agree it doesn't help that it's just one (relatively niche) assembler that has this issue. Ideally I'd look at existing assembly code for this target to see how this change would affect people, but GitHub search doesn't make it easy to filter that granularly, and it's also unlikely that's there's much code for this target to start with.

[alexcrichton]

To be clear I have zero experience with this assembler, so I'm not sure what the best route is! Is the syntax different enough that you wouldn't try to shove multiple architectures/platforms into one file? If that's the case then if you're already not trying to rely on any -D thing in your code then I think it might make sense to simply ignore it since no one familiar with the assembler really relies on its existence anyway.

[Stammark]

Agree with everything that has been said here.

This quite a niche case of someone wanting to compile assembler for ARM on windows, for windows, exclusively through an MSVC environment (where this would be the only assembler available to them).

For context this came in because e.g.

• Crate C contains armasm.asm file
• The file does not contain -D macros
• The crate compiles fine with no -D macros
• The crate does cc::Build::new() and expects it to work out of the box.
• Project P depends on crate C
• Project P has an environment variable or makefile variable to set ASM_FLAGS or something like that
• Project P has no knowledge of the internals of crate C and expects to be able to cross-compile it to Arm, because crate C appears to have support.
  --> Project P can no longer compile

(and that's almost the case of rustc and the psm crate rn :P )

I guess the slightly more "middle of the road" solution would be to emit a warning: If the user did try to use a -Dmacro in their armasm assembler, they would get an error anyway, either when assembling or a "symbol not found" when linking. Then they'd be able to see the warning and realise what's happening.
If on the other hand, if they weren't using the macros, they (and also any project using that crate as a dependency, even if it is 5 dependency layers down), would get a ping of "something fishy is be happening down there".

[alexcrichton]

Hm @Stammark if what you're saying is right then I don't think this PR will help that use case. This is only avoiding flags explicitly defined with .define("A", Some("B")) or something like that (basically those defined in the build script itself). If -D flags are injected via env vars then this PR isn't excluding those from the flags passed to the assembler.

[Stammark]

Ah yes, having actually looked at this properly now, that's correct, my bad!

The env var side would be a different topic entirely (and would be a niche case of a niche case), so ignore most of the above, sorry haha :')

In light of that, I don't have any preference for how to handle this.

All I can think of adding rn is: if we do choose to error, then we could do it gracefully rather than letting the tool derp out on us (but I wouldn't say this is a big deal either :D )

[alexcrichton]

I'm personally ok with ignoring the -D flags here, but @tleasor to confirm, you're sure this would fix your use case too? Which is to say that you've got .define(...) calls but they're intended for other platforms, and it's just convenient to have them on all platforms rather than "everything but aarch64-msvc"?

[tleasor]

@alexcrichton Yes, in the case that led me here assembly files are split by architecture and OS so this fixes that for MSVC ARM targets (the MSVC ARM assembler also seems to be in a superseded format, so is unlikely to ever be mixed with ARM assembly from other platforms).

The solution I'm leaning towards at the moment is to ignore -D flags for these targets as this PR currently does, but also output a message to stderr warning that this is the case. That way projects will still build but also be made aware of potential issues. Would that be a satisfactory way forward?

[alexcrichton]

Seems reasonable to me!

[tleasor]

@alexcrichton Thanks, this should be good to go now.

[alexcrichton]

👍