-
Notifications
You must be signed in to change notification settings - Fork 831
Make FCS build work #3788
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.
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
Make FCS build work #3788
Conversation
|
I will try my best ... |
|
@forki -- Tip: if you want to ran a specific test use: |
|
@KevinRansom I'm not sure but somehow other builds went red as well. I'm deeply sorry, but there was no way to test in pr and now I have to leave. If things are red on master apart from FCS build then please revert my stuff and I retry on monday |
|
If only FCS ist Red then WE can probably ignore for now |
|
It seems that only FCS is red here, so I think it's safe to ignore until this gets worked out |
|
@forki @cartermp I ain't bothered ... I was assuming if there was an _fcs red, I was going to consider things green ... until the _fcs stuff is working. Anyway ... broken builds is how we know work is happening ... Hmmmm!!!! do they say really that Kevin, or did you just make that up? |
|
OK the weird thing is: this looks as if my last groovy change never happened. Wtf |
|
Seriously it should have run ./fcs/build.cmd and not with |
|
@dotnet-bot test this please |
|
#3790 did not run the fcs build. And this one runs outdated version. @KevinRansom do you understand this? |
|
@forki I think it was this: PR: https://github.com/Microsoft/visualfsharp/pull/3781/files I think the first } is I=in the wrong place. The CI will not update the groovy script if it has syntax errors, which makes sense if you think about it a bit. |
|
@dotnet-bot test this please |
|
There you go fixed it. |
|
Thank you Kevin. Now I think I can try to make it work |
|
Ok progress. It now fails on the unit tests. Any ideas @vasily-kirichenko @dsyme @ncave @enricosada @Krzysztof-Cieslak |
maybe the relative path is the issue? If the script is executed in the root, then it would try to look outside the repo. I always use the pattern let cd = __SOURCE_DIRECTORY__
ToolPath = cd @@ @"..\packages\NUnit.Console.3.0.0\tools\nunit3-console.exe"in my build scripts. |
|
I'd appreciate a pull request against my fork. Please please please
Am 21.10.2017 15:55 schrieb "Lukas Rieger" <[email protected]>:
… 05:45:32 Finished Target: Build.NetFx
05:45:32 Starting Target: Test.NetFx (==> Build.NetFx)
05:45:32 ..\packages\NUnit.Console.3.0.0\tools\nunit3-console.exe "--noheader" "--timeout=1200000" "--work=/mnt/j/w/Microsoft_visualfsharp/master/release_fcs_ubuntu14.04_prtest/Release/fcs/net45" "/mnt/j/w/Microsoft_visualfsharp/master/release_fcs_ubuntu14.04_prtest/Release/fcs/net45/FSharp.Compiler.Service.Tests.dll"
05:45:32 mono "..\packages\NUnit.Console.3.0.0\tools\nunit3-console.exe" "--noheader" "--timeout=1200000" "--work=/mnt/j/w/Microsoft_visualfsharp/master/release_fcs_ubuntu14.04_prtest/Release/fcs/net45" "/mnt/j/w/Microsoft_visualfsharp/master/release_fcs_ubuntu14.04_prtest/Release/fcs/net45/FSharp.Compiler.Service.Tests.dll"
05:45:32 Cannot open assembly '..\packages\NUnit.Console.3.0.0\tools\nunit3-console.exe': No such file or directory.
05:45:32 Running build failed.
https://github.com/Microsoft/visualfsharp/blob/
f375d8d/fcs/build.fsx#L90-L96
maybe the relative path is the issue? If the script is executed in the
root, then it would try to look outside the repo.
I always use the pattern
let cd = __SOURCE_DIRECTORY__
ToolPath = cd @@ @"..\packages\NUnit.Console.3.0.0\tools\nunit3-console.exe"
in my build scripts.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#3788 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADgNJzhstVvLitWiD-jqGqfNMQSZ_2gks5sufe-gaJpZM4QBKDJ>
.
|
done. Looking through the build script, relative paths are used in a few other places, so I'm no longer sure this is the issue, but at least it would now print the full path it it is looking for. The Windows build fails with Just judging from the file name, Best solution would be a nuget package providing it. Edit: the dll is checked in so it must probably "only" be copied to the output path. Or the directory with it needs to be added to the dll search directories. |
|
Whatever we need to shave this yak. I appreciate every help!
Am 21.10.2017 16:14 schrieb "Lukas Rieger" <[email protected]>:
… I'd appreciate a pull request against my fork. Please please please
done. Looking through the build script, relative paths are used in a few
other places, so I'm no longer sure this is the issue, but at least it
would now print the full path it it is looking for.
------------------------------
The other build fails with
05:37:00 1) Error : FSharp.Compiler.Service.Tests.ProjectOptionsTests.Project file parsing -- 2nd level references
05:37:00 System.Exception : Could not load file or assembly 'Microsoft.Build.Utilities.v12.0, Version=12.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' or one of its dependencies. The system cannot find the file specified.
05:37:00 at ***@***.***(DateTime loadedTimeStamp, FSharpRef`1 logMap, ProjectOptions opts) in D:\j\w\release_fcs_w---49a16363\fcs\FSharp.Compiler.Service.ProjectCracker\ProjectCracker.fs:line 22
05:37:00 at Microsoft.FSharp.Compiler.SourceCodeServices.ProjectCracker.GetProjectOptionsFromProjectFileLogged(String projectFileName, FSharpOption`1 properties, FSharpOption`1 loadedTimeStamp, FSharpOption`1 enableLogging) in D:\j\w\release_fcs_w---49a16363\fcs\FSharp.Compiler.Service.ProjectCracker\ProjectCracker.fs:line 86
05:37:00 at FSharp.Compiler.Service.Tests.ProjectOptionsTests.Project file parsing -- 2nd level references() in D:\j\w\release_fcs_w---49a16363\tests\service\ProjectOptionsTests.fs:line 180
Just judging from the file name, Microsoft.Build.Utilities.v12.0 is
probably a component of Visual Studio 2013. Maybe this version is not
installed on the buildserver? It probably only has VS2017, and maybe 2015.
Best solution would be a nuget package providing it.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#3788 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADgNCocziXOPWcYStR2fjkLQ_UqK22jks5sufxEgaJpZM4QBKDJ>
.
|
|
The Ubuntu error is now different ... This may be a hint: Maybe need to update mono? |
|
After setting |
|
AHA
So the ProjectCrackerTool SOs. |
|
Are you kidding me?
Am 21.10.2017 19:19 schrieb "Lukas Rieger" <[email protected]>:
… AHA
Process is terminated due to StackOverflowException.
So the ProjectCrackerTool SOs.
@forki <https://github.com/forki>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3788 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADgNF5zwDE_wA9MGcZc1oGLNQFSnBzvks5suieSgaJpZM4QBKDJ>
.
|
c2d5d2f to
83cfd78
Compare
|
@forki are you feeling okay? 😄 For real though, good work. |
|
@forki, what is the meaning of life? |
KevinRansom
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Krzysztof-Cieslak
@forki spent all weekend on this, he may not be qualified to answer that question.
fcs/build.fsx
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this line correct?
+#endifests yet")
looks odd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes merge errors
|
This is ready. The last error is fixed by groovy change in https://github.com/Microsoft/visualfsharp/pull/3805/files |
|
OMG It's green. brb Rebasing..... |
7f4d7e4 to
1de0471
Compare
|
@dotnet-bot test Windows_NT Release_ci_part2 Build please |
KevinRansom
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
I will merge it when I get in tomorrow. Thanks for this, nice job. |
|
Just a suggestion, on your commit, these lines: can be simplified to: The main reason is the condition of |
|
@eriawan these lines are auto-generated by paket. we don't want to touch them! |
@forki ah, ok. thanks for letting me know. 👍 |
|
I will merge this shortly, we are in escrow, and have been trying to figure out what we needed from master and would be allowed to merge for the shipping branch. We ended up going with everything, which is nice :-) When we have completed that, I will pull this and the other fcs pr. Kevin |
|
Yes no need to merge this one. It should be reviewed carefully. But please
consider to merge that groovy change in the other pr. It would unblock
downstream immediately.
Am 24.10.2017 22:27 schrieb "Kevin Ransom (msft)" <[email protected]
…:
@forki <https://github.com/forki>
I will merge this shortly, we are in escrow, and have been trying to
figure out what we needed from master and would be allowed to merge for the
shipping branch.
We ended up going with everything, which is nice :-) When we have
completed that, I will pull this and the other fcs pr.
Kevin
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3788 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADgNNsNkvD51HzMYwrxez4LX6Pdkwymks5svkgcgaJpZM4QBKDJ>
.
|
|
@forki What parts were you concerned about for review? I glanced over and it all looked reasonable - or at least localized to FCS and especially the project cracker |
|
Yes changes in project cracker make me nervous. But I got rid of the
stackoverflow on type load (which is crazy that it happens at all) and
that's probably a good thing.
Am 25.10.2017 02:38 schrieb "Don Syme" <[email protected]>:
… @forki <https://github.com/forki> What parts were you concerned about for
review? I glanced over and it all looked reasonable - or at least localized
to FCS and especially the project cracker
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3788 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADgNLNi8-FBqMej_1fOolJCiBzMSXYJks5svoMfgaJpZM4QBKDJ>
.
|
* Merge dev15.5 to dev15.6 (#3825) * don't update project info if the source file collection is empty (#3792) * install templates VSIX to a unique directory (#3804) * P2p references (#3777) * P2p references * Fix test * test fix * go faster stripes * new project works better * Re-add debug assert for sourcefiles * Parameterise rc location (#3744) * Fix issues * Merge master to dev15.6 (#3826) * Put nupkgs into artifacts (#3806) * Make FCS build work on Jenkins (#3788)

Remark: If we can get a CI server with MSBuild14 installed then we could enable 8 more tests