Skip to content

Conversation

@dsyme
Copy link
Contributor

@dsyme dsyme commented Feb 15, 2017

This is a catalog of all changes that are in http://github.com/fsharp/fsharp that are not in this repo. (This doesn't include added files)

Note these changes are about what constitutes "The Mono F# Compiler". This packaging of the F# compiler is also the one found in the FSharp.Compiler.Tools nuget package, which is being more widely used in automation these days (and is used as part of the LKG process for bootstrapping this repo), so is kind of important.

I'm running through the changes assessing which ones should be brought back into this repo

  • Change in NuGet.targets for OSX. It's possible this is no longer needed if we update to latest NuGet.targets, but is harmless and we can remove it later when we update if needed.

  • The Mono F# compiler still assumes MSBuild 12.0 for stability and to avoid any nonsense about MSBuild not being installed. Every time we've tried to change this it's been horrible (I find mucking with MSBuild versions really tedious and it's unclear if we care to try to progress the Mono F# compiler on to Mono 14.0 or 15.0) This leads to some project file changes and the use of "MicroBuild" is turned off for Mono (it doesn't work with MSBuild 12.0).

  • FsLexYacc 7.0.4 is needed instead of 7.0.3

  • FSharp.Compiler.Tools.4.0.1.21 instead of FSharp.Compiler.Tools.4.0.1.19

  • The Mono F# Compiler uses strong named binaries using STRONG_NAME_FSHARP_COMPILER_WITH_TEST_KEY and src\fsharp\test.snk for everything except FSharp.Core. In all cases the binaries use version number the same as $(FSCoreVersion)

  • The Mono F# compiler defines CROSS_PLATFORM_COMPILER. We obviously don't want this on all the time, and should probably change it to MONO_FSHARP_COMPILER in any case, but some of the changes are relevant to Mono delivery, I'll list them out below

  • Some things were commented out of FSharpSource.targets because Mono was barfing on them. These have been made conditional. Mono seems OK with them like that

  • When running on Mono the default assembly search path includes mono/4.5 and mono/4.5-api and mono/4.5-api/Facades if the directories exist. This is harmless for this repo since it's protected by runningOnMono

  • There are changes in src/fsharp/FSharp.Build/CreateFSharpManifestResourceName.fs that have been made mono-specific. These can be integrated since they are mono-conditional. The Mono version of the F# compiler and targets has strange issues with resources in subdirectories and that problem may be related to these changes. The targets/task/... logic implementing the treatment of resources in MSBuild is horrible messy with a mess of MSBuild and tasks that differs in the Mono and .NET implementations. We should be careful about integrating this change.

  • FSharp.Build passes its own location as a hint to FSharpEnvironment.BinFolderOfDefaultFSharpCompiler to prevent the need for registry scraping. This is reasonable and necessary for the Mono packaging.

  • src/fsharp/FSharp.Build/Microsoft.Portable.FSharp.targets is adjusted to account for a difference in how portable targets work on Mono

  • src/fsharp/FSharp.Compiler.Server.Shared/AssemblyInfo.fs is adjusted to allow this DLL to be compiled without the VS SDK using #if HAVE_VS_SDK

  • Harmless adjustments in some src/fsharp/FSharp.Core.Unittests/FSharp.Core tests

  • Mono BigInteger had some bugs at one point, these are likely fixed but perhaps not on some of the older versions of Mono we still CI against. These lead to some #if CROSS_PLATFORM_COMPILER. We can fix these up at a later point.

  • Dependency on packages\Microsoft.FSharp.TupleSample.1.0.0-alpha-161121 is dropped - fsharp/fsharp doesn't have dependencies on "myget" and all source needs to be available for Debian.

  • Harmless try/with was added to src/fsharp/ast.fs to catch against an exception that Mono was throwing in XML processing in some rare cases. Don't know if the exception is still happening but we may as well integrate the change

  • Unix-friendly console key binding feature has been added to src/fsharp/fsi/console.fs We may a well integrate this, perhaps conditional under MONO_FSHARP_COMPILER

  • the --gui flag is assumed false on Mono. This is because System.Windows.Forms just doesn't exist in good health on Mono and there is no default windowing-friendly event loop that can be assumed.

  • There are bunch of pretty harmless changes in tests/fsharp/core/* for compiling the tests as part of a single program, rather than lots of little programs. That's how http://github.com/fsharp/fsharp currently runs these tests as "smoke tests" on Mono - party because the FSharp.Tests current test harness assumes tools like peverify exist and are in good shape.

These diffs have not been included in this PR:

  • NO: The Mono FSharpSource.targets includes info for building the versions of FSharp.Core used for monodroid, monotouch, xamarinwatchos, xamarintvos, xamarinmacmobile, xamarinmacfull. @nosami, @directhex and @migueldeicaza may be able to advise if these are all still used by Xamarin tooling, or what will be needed going forward. They are not included in this PR since they are not under test.

  • NO: The Mono F# compiler was adjusted to avoid a dependency on System.ValueTuple. The change won't be integrated - it's not clear why that's necessary - we should get to the bottom of it

  • NO: The Mono F# compiler was also adjusted not to reference System.ValueTuple by default for scripts. The change won't be integrated - it's not clear why that's necessary - we should get to the bottom of it

  • NO:The Mono F# targets file removed <CoreCompileDependsOn>_ComputeNonExistentFileProperty</CoreCompileDependsOn> is removed. I don't think that change is needed.

After this PR we should

  1. Work out what to do with those very few remaining diffs listed above

  2. Adjust the Mono F# compiler to enable -gui if running on Windows

  3. Work out what's going on with resources for the Mono F# Compiler

  4. Integrate the changes for authoring the two community nuget packages from http://github.com/fsharp/fsharp FSharp.Core and FSharp.Compiler.Tools

  5. Consider integrating the remaining changes from http://github.com/fsharp/FSharp.Compiler.Services and the authoring of the nuget package built there.

@KevinRansom
Copy link
Contributor

KevinRansom commented Feb 15, 2017

@dsyme

<snip>
NO: SurfaceArea tests were disabled due to some discrepencies in order of results from Mono GetMember
</snip>
These surface are tests are currently really hard to use anyway. Instead of printing out each line why don't we generate them into a hashset and then just look them all up. And print out the extra entries and the missing missing entries?

@dsyme
Copy link
Contributor Author

dsyme commented Feb 15, 2017

These surface are tests are currently really hard to use anyway.

yeah they kind of suck

Instead of printing out each line why don't we generate them into a hashset and then just look them all up. And print out the extra entries and the missing missing entries?

That would be better but historically we never tried to improve anything in http://github.com/fsharp/fsharp since it was only building up more and more misalignment. Likewise if we try to improve things on this PR then that will create yet another round of conflict when we finally integrate the history back to totally align everything. Best to align by whatever means then improve from there

@jack-pappas
Copy link
Contributor

@KevinRansom @dsyme I made some improvements to the surface area tests a while back while working on FSharp.Core, although I didn't ever submit them (I don't remember why). I'll see if I still have them around somewhere and submit a PR if I do -- my changes made any failures of the test much easier to interpret.

@jack-pappas
Copy link
Contributor

It was easier to find my changes than I thought, so I've submitted a PR with them: #2445

@KevinRansom
Copy link
Contributor

@jack-pappas
Thank you for the PR it's awesome.

@jack-pappas
Copy link
Contributor

@KevinRansom You're welcome, glad it's useful.

@dsyme Is it worth continuing with #2114, or will the macOS builds be fixed by merging these changes in?

@dsyme
Copy link
Contributor Author

dsyme commented Feb 17, 2017

@jack-pappas It is worth continuing with #2114 - we don't use Travis in this repo - and it would be great to have Mono+OSX coverage

@dsyme Is it worth continuing with #2114, or will the macOS builds be fixed by merging these changes in?

@dsyme
Copy link
Contributor Author

dsyme commented Feb 20, 2017

This is now green and ready to merge

@dsyme dsyme changed the title [WIP] reverse integrate all fsharp/fsharp changes reverse integrate all fsharp/fsharp changes Feb 21, 2017
Copy link
Contributor

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants