Skip to content

added os-string flag to cabal file #290

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

added os-string flag to cabal file #290

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fea2782
added os-string flag to cabal file
BebeSparkelSparkel Jan 21, 2024
6b63999
tightened os-string max version bound
BebeSparkelSparkel Feb 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion hashable.cabal
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,13 @@ flag random-initial-seed
manual: True
default: False

flag os-string
description:
Use the new os-string package and filepath pacakage version >= 1.5.0.0

manual: False
default: False
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default: False here interacts badly with --allow-newer. Trying to do any builds that use that will result in our flag being false but using a version of filepath that needs it.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no way around it. It has to be an automatic flag.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it has to be automatic, but can't it be default-true automatic?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it hard to reason about this. Also note that all the other packages have the default as False.

This strategy is also suggested by Cabal-syntax, which we derived this from: https://hackage.haskell.org/package/Cabal-syntax-3.6.0.0

Maybe @Bodigrim knows more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josephcsible if you are already passing --allow-newer you can pass -c 'hashable +os-string' as well.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Bodigrim I know I can do that, but I don't want to have to do that for every single cabal install --allow-newer that I ever run in the future that pulls in hashable.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another idea: if we really want to keep the flag default: False, then could we do what QuickCheck does, calling the flag something like old-filepath, and having True be the legacy path and False be the modern path?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know any particular reason to stick to default: False, but I also find optimizing for --allow-newer to be a weak justification.

Ultimately, I think that

dependency on os-string should be unconditional (and code to be written).

is a better solution, in which case the default value of the flag would be immaterial for --allow-newer.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The solution that was just merged in #293 doesn't cause this problem, so I'm happy with it.


library
exposed-modules:
Data.Hashable
Expand All @@ -84,10 +91,17 @@ library
, bytestring >=0.10.8.2 && <0.13
, containers >=0.5.10.2 && <0.7
, deepseq >=1.4.3.0 && <1.6
, filepath >=1.4.1.2 && <1.5
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd put filepath >= 1.4.1.2 && < 1.6 here and then filepath < 1.5 into the else (and delete filepath from the if part).
This style is slightly more concise. And it is apparent that we always depend on filepath.

, ghc-prim
, text >=1.2.3.0 && <1.3 || >=2.0 && <2.2

if flag(os-string)
build-depends:
filepath >=1.5.0.0 && <1.6
, os-string >=2.0.2 && <2.1
else
build-depends:
filepath >=1.4.1.2 && <1.5

if !impl(ghc >=9.2)
build-depends: base-orphans >=0.8.6 && <0.10

Expand Down