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

Why is 6.0.2 the min FSharp.Core version requirement? #121

Closed
bartelink opened this issue Dec 6, 2022 · 6 comments · Fixed by #123
Closed

Why is 6.0.2 the min FSharp.Core version requirement? #121

bartelink opened this issue Dec 6, 2022 · 6 comments · Fixed by #123
Assignees
Labels
investigation Requires further or deeper investigation question Further information is requested

Comments

@bartelink
Copy link
Member

Great to see this taking shape. Was able to port Equinox to use it in one impl without issues. Am intending to spread that to the other integrations in due course.

However, it does force a dep on FSharp.Core v 6.0.2 - is there an upstream requirement that triggers this ? Is this likely to remain stable?

(Reason I ask is that the Equinox deps are 6.0.0 and I was trying to hold the line on that. Should I up the requirement, Equinox will likely shift to depending on 6.0.6, as there's an ugly shim required for a StackOverflow in Async.Parallel that's not fixed until then)

@bartelink
Copy link
Member Author

bartelink commented Dec 8, 2022

Tried to reduce the dependency:

Would still be interested to know the nature of the hard requirement for 6.0.2...

@abelbraaksma
Copy link
Member

I tried the “earliest version possible”, and the NoEagerConstraintApplication is a requirement, the overload resolution won’t work without it.

The earliest I could make work with everything in the resumable code was 6.0.2, but I’ve changed quite a few things since the first TaskSeq versions, it’s possible I removed something that doesn’t require that anymore. I’d have no problem making the dependency on Core a lower version. In fact, that’d be a plus.

I’ll have to dive into history though, hopefully I recorded the ‘why’ of the decision.

@abelbraaksma
Copy link
Member

abelbraaksma commented Dec 10, 2022

It was done as part of #87, and from the text, it suggests I tried to go as low as possible. Let me create a quick PR and see what happens if I use 6.0.1.

See #123, running now.

@abelbraaksma abelbraaksma self-assigned this Dec 10, 2022
@abelbraaksma abelbraaksma added question Further information is requested investigation Requires further or deeper investigation labels Dec 10, 2022
@bartelink
Copy link
Member Author

Thanks for looking - tbh if I'd seen the need for the attribute, I probably wouldn't have asked in the first instance, but I do appreciate you investigating and the fact that we now have a paper trail ;)

While rippling dependency updates are not ideal, it's clear that there's a reason so this is not a real problem. It's not inconceivable that there are ancillary perf and stability benefits to 6.0.2, so once the option of targeting 6.0.0 is gone it's less critical.

In Equinox and Propulsion, I've since bitten the bullet and upped their min req to 6.0.7 (in order to ensure 6.0.6's fix for the Async.Parallel stack overflow edge case is forced in)

Happy for this to be closed as and when you see fit.

@gusty
Copy link
Member

gusty commented Dec 10, 2022

I think I can update the code and make the overload resolution work without needing the NoEagerConstraintApplication attribute.

The problem of using that attribute is worst than just forcing everyone to 6.0.2. AFAIK that attribute won't be recognized by earlier F# compilers.

@abelbraaksma
Copy link
Member

abelbraaksma commented Dec 11, 2022

Good points, @gusty. But considering that task uses the same attribute in FSharp.Core, and this library cannot be used without task, the backward compat issue with earlier compilers is irrelevant. Same is true, for instance, for the resumable code in this lib, which isn't supported either by earlier compilers.

@bartelink I've downed the version to 6.0.1. I may reconsider at some point to go to 6.0.0, but that requires a different approach for the overload resolution that I'm not to keen on trying.

This will be part of whatever the next version is (either 0.3.1 or 0.4.0).

@abelbraaksma abelbraaksma added this to the v0.4.0-alpha.1 milestone Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
investigation Requires further or deeper investigation question Further information is requested
Projects
None yet
3 participants