Fix expressions for provided method calls #472
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@rneatherway Heads up that I'm seeing these failures that seem unrelated to this checkin. Not sure what's going on. |
|
@dsyme yes, this is Mono 4.2 |
|
Hi @rneatherway, I've got some really strange problem with project cracking on Windows in this PR. I'm cracking TestProject.fsproj. This has a project reference to The problem is that on my local machine this generates a reference to the Release TestTP, but on the AppVeyor CI server it gives a reference to the Debug TestTP. Do you know of anything that might cause this behaviour? I'm cracking with the code below |
|
If the problem remains after the merge with master I suggest trying with logging on. You can just run the cracker from the command line e.g.: And then paste the output here if it isn't immediately helpful. |
|
Log below. The version on my local machine has Debug v. Release inconsistencies too so I'm using that. Note that we have: and Full log: |
|
This is a little bit scary, but probably not the problem: |
|
Yes. peverify on the .exe gives: I think an assembly can't have |
|
I changed the extension to "Tool" and the peverify error went away. But the very strange MissingMethodException didn't go away. Why would the |
|
Oh it is this: https://github.com/fsharp/fsharp/blob/master/src/fsharp/FSharp.Build/Fsc.fs#L375 This was some hack the Visual F# Tools 2.0 team put in. Goodness knows why. It's triggering because we're using the settings that make FSharp.Build.dll think it's being used from inside Visual Studio. |
|
OK I have some vague idea what is happening. The first Configuration=Release is parsing TestTP, then TestProject, which as a subjob also ends up doing something for TestTP again (this is the last Configuration=Debug). I found that subjob includes: I honestly don't know why this is happening. My usual next time is grepping through a checkout of MSBuild/XBuild for "Removing Properties". It does occur to me that if the subproject is using the correct Configuration then if we use the same code path I introduced for Mono (in convert) to inject the referenced projects rather than the ChildProjectReferences (in Parse) then it should give the correct answer. However, this seems hackish. |
|
The HostCompile trick is great (in its effect if not implementation) because it lets us run a full compile without being really slow, which is really the only way to properly simulate what the compilation will be (and therefore what the references should be). |
|
So this works: The properties have to be global, and we need the highly obvious ShouldUnsetParentConfigurationAndPlatform. |
:-) I wonder why the project isn't thought to be resolved though? I'm thinking of this comment that you linked to:
|
|
Ah, the comment further down says:
|
|
So I suppose this is an artefact of us parsing outside of a solution context. |
|
I guess so. What was very strange though was that we got inconsistent Debug and Release information back. |
|
But it does indicate that setting |
Do you mean one of each for the referenced project? The reason is that one is from MSBuild deciding to parse it itself and one is from us deliberately choosing to parse it as a new root with the properties set in order that we can provide the ReferencedProjectOptions.
Yes, but I remember it being important for something, and it's used by the Visual F# integration. I can't easily search from this computer though. I think it might stop it trying to compile referenced projects for example. |
Right, it's needed to activate the |
| @@ -33,10 +31,10 @@ type ProjectCracker = | |||
| arguments.Append(' ').Append(enableLogging.ToString()) |> ignore | |||
| for k, v in properties do | |||
| arguments.Append(' ').Append(k).Append(' ').Append(v) |> ignore | |||
|
|
|||
| let codebase = Path.GetDirectoryName(Uri(typeof<ProjectCracker>.Assembly.CodeBase).LocalPath) | |||
There was a problem hiding this comment.
I'm a little unclear on the difference here. Does one or the other work even in the presence of shadow-copying?
There was a problem hiding this comment.
That's right, Location gets the copied location, CodeBase gets a URI to the original location.
|
Looks like a good cleanup. Ready to merge if you're done. |
|
Yes ready to merge |
|
BTW some questions - when using ProjectCracker from FSI.EXE I found I needed to add a reference to ProjectCracker.Tool.exe too, to allow the binary serialization to work. Could we switch to JsonDataContract serialization to avoid any rigid type dependencies? |
|
Also, have you checked what happens when the user references the projectCracker nuget package? Does the Tool.exe get added as a reference? Does the .exe.config for Tool.exe get added too and carried along with the DLL and the EXE? |
Yeah I can change it back to the JsonDataContract. I'll do another PR after merging this one.
No, I'm not sure how to test it without uploading to nuget.com. What do you mean by referencing a NuGet package? Adding it to paket.references and telling it to automatically modify project files? Maybe VS has some magic it likes to do as well? |
|
There's a CI nuget feed here: https://github.com/fsharp/FSharp.Compiler.Service/#nuget-feed |
dsyme
added a commit
that referenced
this pull request
Dec 3, 2015
Fix expressions for provided method calls
|
OK merged. The release notes are updated with a 2.0.0.0-beta tag. (FWIW I think we should start to adopt SemVer for this repository as it seems some people are starting to rely on binary compat) |
|
When you're ready feel free to push a nuget package to nuget.org |
|
OK I will change it back to the JSON serialization first, shouldn't take long as I have the other branch around. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #460.