F# Android projects don't build when language is changed in XS fixes …

[nosami]

F# projects don't build when lang is changed in XS

Setting the UI language in XS sets the msbuild variable LANGUAGE = en
(or whatever language you choose).

The Android F# targets check for the existence of this variable to
determine if Microsoft.FSharp.targets has been loaded.

Microsoft.FSharp.targets sets <Language>F#</Language> so it is better
to check for the presence of F# being set rather than checking to see
if it's not empty.

Fixes #43525

[jonpryor]

Please improve the commit message.

What is $(Language)? What sets $(Language)? Why is it better to compare $(Language) to to F# vs. the empty string?

What does "when language is changed in XS" mean? Programming language? Human translation language? Something else?

6 months from now, how are we going to understand the intent behind this change?

All of that should go into the commit message.

[nosami]

Yeah, sorry. It was late. Fixed comment.

[jonpryor]

Thank you for the updated commit message.

It now raises more questions. :-)

Setting the UI language in XS sets the msbuild variable LANGUAGE = en

1. Why is the UI language exported as the $(Language) MSBuild property?!
2. Ignoring (1), if $(Language) is a human language that the IDE is displaying, _why is the $(Language) property used _at all* in the .targets file?*.

The Android F# targets check for the existence of this variable to
determine if Microsoft.FSharp.targets has been loaded.

Why is this necessary? To prevent multiple inclusion of the .targets file?

If the IDE is using $(Language) for one purpose, trying to re-use $(Language) for another purpose seems doomed. (How does this not break the F# targets in the first place, even outside of Xamarin.Android?)

---

Here's a concern/scenario: MSBuild properties specified on the command line cannot be overridden in .targets files.

Consider this simple MSBuild project, which

1. Loads Microsoft.FSharp.Targets twice, but
2. The second load is "protected" by a '$(Language)' == '' check to prevent the second inclusion

The file is named, for example purposes, as xa-175.targets:

<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <Import
      Project="$(MSBuildExtensionsPath32)\..\Microsoft SDKs\F#\4.0\Framework\v4.0\Microsoft.FSharp.Targets" />
  <Import
      Condition="'$(Language)' == '' And Exists('$(MSBuildExtensionsPath32)\..\Microsoft SDKs\F#\4.0\Framework\v4.0\Microsoft.FSharp.Targets')"
      Project="$(MSBuildExtensionsPath32)\..\Microsoft SDKs\F#\4.0\Framework\v4.0\Microsoft.FSharp.Targets" />

  <Target Name="Build" />
</Project>

Build it, and it's fine:

$ xbuild xa-175.targets /nologo /v:minimal
# no output

Override the Language property, and it's not so fine:

$ xbuild xa-175.targets /nologo /v:minimal /p:Language=
/Users/jon/tmp/xa-175.targets:  warning : Cannot import project '/Library/Frameworks/Mono.framework/Versions/4.4.2/lib/mono/xbuild/../Microsoft SDKs/F#/4.0/Framework/v4.0/Microsoft.FSharp.Targets' again. It was already imported by '/Users/jon/tmp/xa-175.targets'. Ignoring.
/Users/jon/tmp/xa-175.targets:  warning : Cannot import project '/Library/Frameworks/Mono.framework/Versions/4.4.2/lib/mono/xbuild/../Microsoft SDKs/F#/4.0/Framework/v4.0/Microsoft.FSharp.Targets' again. It was already imported by '/Users/jon/tmp/xa-175.targets'. Ignoring.

We now have warnings, because the condition on the second import wasn't what we thought it was, because properties specified on the command-line are immutable.

To bring us full circle, how is XS specifying the $(Language) MSBuild property? Is it as a "normal" MSBuild property, within some .props file that's included? Or is it specified as a command-line argument, or whatever the MSBuild embedding API equivalent is?

I have no idea, but to me this means that you cannot rely on the $(Language) property. At all. It could be set when the F# targets haven't been imported, meaning you'll get compiler errors due to missing ~everything, or it could always be clear, meaning you'll import the projects file too many times.

[nosami]

Yes. I agree that this is a kludge but I don't see an easy solution. The problem is that MSBuild takes input from all environment variables.

$(Language) is a known MSBuild variable used by both C# and F# targets. It's also a very common environment variable on Unix and also happens to get set by XS to specify the user's preferred UI language.

The build breaks if the Microsoft.FSharp.targets file is imported twice.

Microsoft.FSharp.targets imports the following properties

    <PropertyGroup>
        <MSBuildAllProjects>$(MSBuildAllProjects);$(MSBuildThisFileFullPath)</MSBuildAllProjects>
        <DefaultLanguageSourceExtension>.fs</DefaultLanguageSourceExtension>
        <Language>F#</Language>
        <TargetRuntime>Managed</TargetRuntime>
        <Tailcalls Condition="'$(Tailcalls)'==''">$(Optimize)</Tailcalls>
        <FrameworkRegistryBase Condition="'$(TargetFrameworkIdentifier)'=='Silverlight'">Software\Microsoft\Microsoft SDKs\$(TargetFrameworkIdentifier)</FrameworkRegistryBase>
        <!-- Visual studio requires a non-empty RootNamespace value for "Add New Item" to work. -->
        <RootNamespace Condition="'$(RootNamespace)'==''">RootNamespace</RootNamespace>
        <Actual32Bit Condition="'$(TargetFrameworkVersion)'=='v2.0' or '$(TargetFrameworkVersion)'=='v3.0' or '$(TargetFrameworkVersion)'=='v3.5' or '$(TargetFrameworkVersion)'=='v4.0'">false</Actual32Bit>
        <Actual32Bit Condition="!('$(TargetFrameworkVersion)'=='v2.0' or '$(TargetFrameworkVersion)'=='v3.0' or '$(TargetFrameworkVersion)'=='v3.5' or '$(TargetFrameworkVersion)'=='v4.0')">$(Prefer32Bit)</Actual32Bit>
    </PropertyGroup>

I'm no expert in MSBuild so open to other ideas but it seems that we need to detect one of these to know if it's already loaded.

[jonpryor]

The problem is that MSBuild takes input from all environment variables.

Yes, but environment variables aren't the same as command-line arguments!

$ LANGUAGE= xbuild xa-175.targets /nologo /v:minimal
# no output

If XS is exporting the LANGUAGE environment variable, that's fine, afaict. If it's exporting an immutable MSBuild property, that'll break stuff.

Which is XS doing?

If XS is exporting an environment variable, are we sure we understand this bug correctly, and that it's not something else?

If XS is exporting an immutable MSBuild property, why is XS exporting an immutable MSBuild property? Can that be changed?

[nosami]

//cc @mhutch advice please

[jonpryor]

Here's another plausible fix. This is in /Library/Frameworks/Mono.framework/Versions/4.4.2/lib/mono/4.5/Microsoft.FSharp.Targets:

<Import Project="$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\$(MSBuildThisFile)\ImportBefore\*" Condition="'$(ImportByWildcardBeforeMicrosoftFSharpTargets)' == 'true' and exists('$(MSBuildExtensionsPath)\$(MSBuildToolsVersion)\$(MSBuildThisFile)\ImportBefore')"/>

Mono.framework could add a file here which imports a new, unambiguous, MSBuild property, such as $(ProgrammingLanguageName), and this new MSBuild property could be used by Xamarin.Android instead of using $(Language).

Alas, that would be Mono-specific.

[nosami]

XS is exporting an environment variable but MSBuild converts all env vars into MSBuild vars. See here for a diagnostic build log (containing this fix)

[mhutch]

Maybe I can explain it more clearly.

1. MSBuild initializes its property collection with all of the environment variables in the current environment. This is a bad design decision and has caused major problems but we have to live with it.
2. Gettext uses the LANGUAGE environment variable to specify the current language, so XS sets this when users set a custom UI language. As a consequence of 1 it is imported into MSBuild.
3. The fact that XS sets the LANGUAGE env var is irrelevant - on a Linux system it would be set by the system.
4. The Common targets expect a Language property to be set by the language targets (F#, C#, etc).
5. Although a user could set the value of the Language property in their project or on the command-line, this would be ignored, as the targets override it regardless of whether it already had a value: <Language>F#</Language>
6. My patch - which this is fixing - made an incorrect assumption that if the Language had not been set, then the F# targets had not yet been imported.
7. We do not control the F# targets (at least on Windows) so cannot put a new "unambiguous" property (such as HaveFSharpTargetsBeenImported) in them.

So - I see 2 possible solutions:

a. Instead of checking whether Language is empty, check whether it is not F#. This is IMO a very robust solution, as there is no reason I know of that it would ever be externally set to this value.

b. Check some other property that is set by the F# targets. I didn't see a good candidate though.

[edited for accuracy]

[jonpryor]

\6. Although a user could set the value of the Language property in their project or on the command-line, this would be ignored, as the targets override it regardless of whether it already had a value: F#

This is incorrect, as demonstrated above. Command-line provided parameter values are immutable, and cannot be overridden, by anything.

b. Check some other property that is set by the F# targets. I didn't see a good candidate though.

I didn't see a good candidate either. :-(

[jonpryor]

Merged with a more "elaborate" commit message.

[mhutch]

Thanks for the correction. Making vars from the command-line immutable also seems like a weird design decision :)

Property from env vars are definitely mutable though.

[nosami]

Great. Thanks!