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.

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

Update Proj-Info and FCS to support F# 6 and .Net 6 #846

Merged
merged 50 commits into from
Nov 8, 2021

Conversation

baronfel
Copy link
Contributor

@baronfel baronfel commented Nov 1, 2021

No description provided.

@baronfel
Copy link
Contributor Author

baronfel commented Nov 1, 2021

Carries on from #825

@baronfel
Copy link
Contributor Author

baronfel commented Nov 2, 2021

10 tests failing on macos/ubuntu, and those are mostly the go-to-def tests. Window's tests are horked, possibly due to script project options discovery?

@baronfel baronfel changed the title fix CI for proj-info update? Update Proj-Info and FCS to support F# 6 and .Net 6 Nov 6, 2021
This was referenced Nov 6, 2021
@baronfel
Copy link
Contributor Author

baronfel commented Nov 6, 2021

This seems to be working under manual testing, but the automated tests are (largely) failing due to typeload exceptions in msbuild. I believe this is due to the hosting model the tests use - instead of testing out-of-process spawned FSAC instances at the LSP level, we have a project-level reference, and then dlls are xcopied, etc, etc. It's a mess.

Therefore, given the imminent release of F# 6 and .Net 6, I propose the following set of steps:

  • manually verify the existing automated test threads on
    • windows
    • macos
    • ubuntu
  • merge this if manual testing is good
  • release a new package + ionide in time for release
  • immediately turn around and start work on making the tests spawn out-of-proc LSP servers as a foundation for future work

@baronfel
Copy link
Contributor Author

baronfel commented Nov 8, 2021

This is starting to look really good. I figured out a couple core problems with how project options were being generated/modified in ways that got them out of sync with the project options used as keys in certain compiler caches, and as soon as I fixed those a lot of the inability to find parse and check results went away. Then it's been a matter of chipping away at implementation details and differences between FCS 39 and 41.

There are roughly 15 tests still failing, and they fall into the following categories:

  • open <namespace> insertions in complex situations - this has changed materially and the logic we use needs a go-over in complex situations, like heavily-nested modules. The one pain point remaining is the amount of indent required, which should be looked up by checking the indent of the declaring statement (in the 'nearest' case) and using that, which I think will require a syntax tree walk to do. this is IMO not critical to solve before release.
  • rename tests - these have some sort of subtle timing or state pollution happening in the tests. I'm tempted to change these tests to be truly isolated to rule that out. these work while under the debugger, which is what makes me think this
  • find references test - similarly this works under test, so I may want to isolate this one as well.

I'd like to try to solve the rename and find references testing issues before release, and of course do manual testing (@Krzysztof-Cieslak and I have had various issues with scripts when testing this work locally that doesn't align with the overall successful test runs here).

@baronfel
Copy link
Contributor Author

baronfel commented Nov 8, 2021

We've done manual testing, and we're happy with the state of this. Work will need to be done subsequently to green up the rename and find references features, but we feel that supporting F# 6 and .Net 6 more correctly is worth the loss of those features for a short amount of time.

@baronfel baronfel merged commit f176825 into ionide:main Nov 8, 2021
@baronfel baronfel deleted the update-proj-info branch November 8, 2021 16:45
@adelarsq
Copy link
Contributor

adelarsq commented Nov 8, 2021

@baronfel A huge thanks for your work on this! Good job! 👏🏻🔷🚀

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.

3 participants