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

Support filepath >= 1.5.0.0 and os-string #305

Merged
merged 3 commits into from
Nov 29, 2023
Merged

Support filepath >= 1.5.0.0 and os-string #305

merged 3 commits into from
Nov 29, 2023

Conversation

hasufell
Copy link
Member

No description provided.

@vdukhovni
Copy link

Under what conditions is the new os-string flag set?

@Bodigrim
Copy link
Contributor

@vdukhovni it's an automatic flag, Cabal solver will figure it out depending on a required build plan.

@vdukhovni
Copy link

So like the cabal-syntax flag that's been me grief recently, right? :-)

@vdukhovni
Copy link

vdukhovni commented Nov 26, 2023

Should any of the tests insist on the flag being set? Or does that already happen automatically for some of them, based on their extant "build plan"? (By tests here I mean CI runs)

@hasufell
Copy link
Member Author

So like the cabal-syntax flag that's been me grief recently, right? :-)

I've never done this sort of stuff, so I'm not sure about all the consequences.

What exactly caused grief?

@hasufell
Copy link
Member Author

Should any of the tests insist on the flag being set? Or does that already happen automatically for some of them, based on their extant "build plan"? (By tests here I mean CI runs)

Yeah, should probably add that.

@Bodigrim
Copy link
Contributor

What exactly caused grief?

See haskell/cabal#8370.

A downstream package can end up with a build plan containing both filepath-1.4 and os-string-2.0. If it's not careful enough, it easily runs into conflicting identifiers and hard to debug build errors.

That said, I think we are in a better position than hackage-security here: unix is to depend on filepath in both branches, and CPP conditions are in terms of #if MIN_VERSION_filepath(1,5,0) as opposed to #ifdef MIN_VERSION_os_string.

@hasufell
Copy link
Member Author

Yeah, packages that want to work across many GHC versions and use OsString will have to pay a toll.

We may need a better migration guide.

@hasufell hasufell merged commit 727853d into master Nov 29, 2023
43 checks passed
@vdukhovni
Copy link

I'm a bit concerned about the flag being inappropriately chosen by the solver, causing issues similar to what I see with older cabal-install builds. Are we sure the ecosystem will handle the flag gracefully?

If the GHC team is on the verge of cutting 9.6.4, I'd actually prefer to not include this change so late in the process of creating a bug-fix GHC release. It should instead appear in a future 9.10.1 release, with lots of previous CI runs.

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