-
Notifications
You must be signed in to change notification settings - Fork 777
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
Attempt to make FCS solution build without arcade and with the SDK specified in global.json #14677
Attempt to make FCS solution build without arcade and with the SDK specified in global.json #14677
Conversation
…ecified in global.json
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.
Should we update DEVGUIDE with this being available and how to use it?
…kii/fsharp into fcs-no-proto-no-arcade
@vzarytovskii could we have |
It seems it passes. Can someone please try it on different OSs? Just |
@vzarytovskii as mentioned on Slack, I believe the test SDK is missing: I also am not seeing any files in ComponentTests: |
Thanks, will add it and test in IDEs |
Regarding which projects to keep: It should include all projects which are likely to be affected (=> get broken, tests start failing) by a contribution. Otherwise: |
FCS solution is about faster devloop, so if something else besides the compiler lib is broken, one should go the standard buildscripts way. |
Test projects and even benchmarks are included in that solution as of today anyway. |
Isn't that always a bit of how it has been? When I change something in the compiler, I for sure don't open I typically never run all the tests, to be honest. I don't have a good experience with it, some tests will fail on my machine while they run fine on the CI. #14637 is a good example of that. |
I don't run the full suite locally either. It's more about the ability to locate a test (given the name from a failing CI job) and fixing it in editor. |
I think FCS lib, tests and benchmarks are fine for now. I may want to figure out a way of debugging tests using vsdbg and netcoredbg. |
Slightly related:
Can we replace multiple solutions with a single one + solution filters?
Those can be built with `dotnet` just as well. And if solution folders are
setup nicely, it's easy to add missing projects. Although need to make it
so that no local filter changes end up in git.
…On Tue, 31 Jan 2023, 11:32 Vlad Zarytovskii, ***@***.***> wrote:
I think FCS lib, tests and benchmarks are fine for now. I may want to
figure out a way of debugging tests using vsdbg and netcoredbg.
|
I'm not sure that FSAC supports solution filters, so that might regress the devcontainers development scenario. |
Solutions is the official suggestion from engineering system team, so we'll probably be sticking with multiple solutions for now. |
One problem left - is that our test framework relies on where test dlls are emitted to, and we don't change it in case if arcade-less build. |
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.
This is a super cool, can't wait to start using these changes! 🎉
I've just tried working with this branch, and building FCS is a fantastic experience there: all builds super fast and stable. I couldn't make any tests work, though, and get this error:
And if I try to do
|
So two issues - test framework relies on artifacts path. Solution to 1st one is to point output to standart path used by arcade. It will likely fix the second one. |
This is promising, I'd love to have a regular dotnet build, which also works exactly the same in the IDE (on In an ideal world I'd like all of these to work the same way, be able to mix them and not have to recompile when making a switch:
but I understand that's out of scope for this particular PR. Some test results on my box
@nojaf
(the working directory doesn't matter - same result with SDKs installed:
@nojaf What solution do you have open in that case? |
So, arcade is turned off based on the solution name, and won't work with standalone project. You can set an msbduild flag |
I wonder if instead of basing it on the solution name we could base it on an optional user config file which can dictate whether to use Arcade SDK or not. Even though most of the compiler code lives in FSharp.Compiler.Service, one still needs to build/run fsc to invoke it through the standalone compilation endpoint. |
@nojaf @safesparrow @auduchinok Keep in mind, that mixing build scripts and normal build of FCS.sln is not supported, due to magic arcade and proto-based build do. I had to include some more projects to the solution, otherwise, Rider fails to build it. So, main scenarios targeted by this:
If you want/need to dedug full scenario (using build scripts), then you should use Everything should work fine. I've only tried running component tests, but not service tests, so there might be some issues with those. |
Ok, I'm fine with merging it and testing what works/what doesn't. If something goes wrong, it's easy to turn off via user file. @safesparrow @nojaf @auduchinok wdyt? |
Tried it just now. This all works very smoothly for me. I was not able to run:
Outputdotnet publish .\src\fsc\fscProject\fsc.fsproj -c Release -r win-x64 -p:PublishReadyToRun=true -f net7.0 --no-self-contained When I added the I'm in favour of merging this and seeing how it goes. |
Should |
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.
I haven't tested the latest version, but I'll depend on @nojaf's judgement. Happy for this to be merged from the user's perspective.
I did not look at the code changes though.
Works for me with VS and Rider, but not with Ionide/FSAC with the current contents in the Edit: Seems like it was a lucky shot, I can't reproduce a working Ionide/FSAC anymore. But well, I assume that shouldn't be a blocker here. |
Setting |
It might be that Ionide/FSAC do not provide the same MSBuild property pointing to the solution file being open/analysed, thus the auto-enablement of the feature for a certain solution does not get triggered. |
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.
It's super cool to see this going, I'm definitely going to try using this solution!
It should make it much easier to build and test the compiler/service. On the other hand, making it incompatible with the solutions that use the build scripts makes it much harder to debug tests there, and it's often needed when doing some changes that affect VS tests. It's OK if it fix it later, but it still seems to be important to support the scenario where a contributor might want to switch between solutions (i.e. using FCS solution most of the time and only using VisualFSharp.sln when fixing the tests).
vsintegration/src/FSharp.UIResources/FSharp.UIResources_mnhnvqun_wpftmp.csproj
Outdated
Show resolved
Hide resolved
🎉 |
Well, first, we need to wait official build to succeed :D |
…ecified in global.json (dotnet#14677)
Now, when building with plain dotnet, we
If this passes (and overall looks good for @KevinRansom, @auduchinok, @safesparrow and @nojaf), we can further cleanup/change FCS solution to be slimmer/include only relevant tests.