Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix for #2595, ported from fsharp/fsharp #2605

Merged
merged 2 commits into from
Mar 20, 2017
Merged

Fix for #2595, ported from fsharp/fsharp #2605

merged 2 commits into from
Mar 20, 2017

Conversation

jack-pappas
Copy link
Contributor

There's a small change to Microsoft.FSharp.targets from fsharp/fsharp that's required to fix #2595 by embedded (as expected) the FSCore.resources file into the compiled FSharp.Core assembly.

KevinRansom
KevinRansom requested changes Mar 14, 2017
View reviewed changes
Copy link
Member

@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.

Apparently this breaks a bunch of tests.

dsyme
dsyme reviewed Mar 14, 2017
View reviewed changes
src/fsharp/FSharp.Build/Microsoft.FSharp.targets Outdated
@@ -221,7 +221,7 @@ this file.
Prefer32Bit="$(Actual32Bit)"
References="@(ReferencePath)"
ReferencePath="$(ReferencePath)"
Resources="@(_CoreCompileResourceInputs);@(CompiledLicenseFile);@(AdditionalEmbeddedResource)"
Resources="@(_CoreCompileResourceInputs);@(ManifestResourceWithNoCulture);@(ManifestNonResxWithNoCultureOnDisk);@(CompiledLicenseFile);@(AdditionalEmbeddedResource)"
Copy link
Contributor

Choose a reason for hiding this comment

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

@jack-pappas This fix doesn't work on .NET Framework on Windows, perhaps we should make it conditional on Mono

@jack-pappas
Copy link
Contributor Author

@dsyme @KevinRansom I checked the build log for one of the failed builds. In the command line for building FSharp.Compiler.dll with fsc-proto, the resources are specified twice:

CoreCompile:
  D:\j\workspace\release_ci_pa---3f142ccc\src\fsharp\FSharp.Compiler\..\..\..\Proto\net40\bin\fsc-proto.exe -o:obj\release\net40\FSharp.Compiler.dll
  -g
  --debug:pdbonly
  --noframework
  --baseaddress:0x06800000
  --define:MSBUILD_AT_LEAST_14
  --define:STRONG_NAME_AND_DELAY_SIGN_FSHARP_COMPILER_WITH_MSFT_KEY
  --define:INCLUDE_METADATA_WRITER
  --define:INCLUDE_METADATA_READER
  --define:COMPILER
  --define:EXTENSIONTYPING
  --define:TRACE
  --define:VS_VERSION_DEV15
  --define:ENABLE_MONO_SUPPORT
  --define:BE_SECURITY_TRANSPARENT
  --define:FX_LCIDFROMCODEPAGE
  --define:FSI_SHADOW_COPY_REFERENCES
  --define:FSI_SERVER
  --doc:D:\j\workspace\release_ci_pa---3f142ccc\src\fsharp\FSharp.Compiler\..\..\..\release\net40\bin\FSharp.Compiler.xml
  --optimize+
  --resource:obj\release\net40\FSComp.resources
  --resource:obj\release\net40\FSStrings.resources
  --resource:obj\release\net40\FSComp.resources
  --resource:obj\release\net40\FSStrings.resources
...

Maybe we should be handling that in a better way within the Fsc task in FSharp.Build? Also, what is the expected behavior of the compiler when a resource is specified multiple times?

@dsyme
Copy link
Contributor

dsyme commented Mar 15, 2017

@jack-pappas Yes, the fix adds the resources twice on Windows

The overall problem is that the Mono targets are just handling resources the wrong way, as part of working around differences between Mono and Windows targets.

@jack-pappas
Fix .targets to work with xbuild and msbuild
04406d9 
Fix Microsoft.FSharp.targets with a workaround so resources are included
correctly when building with either msbuild or xbuild.
@jack-pappas
Copy link
Contributor Author

@KevinRansom I added a workaround for the issue where the resources were included twice and broke the tests, and all of the CI builds are passing now.

@dsyme
Copy link
Contributor

dsyme commented Mar 20, 2017 •
edited
Loading

This is OK. I really wish we could get to the bottom of these resource processing differences on Mono

@KevinRansom
Copy link
Member

Thanks for this

Kevin

@KevinRansom KevinRansom merged commit 8018564 into dotnet:master Mar 20, 2017
@jack-pappas jack-pappas mentioned this pull request Mar 21, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KevinRansom KevinRansom KevinRansom approved these changes

@dsyme dsyme dsyme approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Mono builds don't produce usable output
4 participants
@jack-pappas @dsyme @KevinRansom @msftclas