-
Notifications
You must be signed in to change notification settings - Fork 85
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.
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
added os-string flag to cabal file #290
Conversation
For the PVP the os-string version range looks too loose. It should be <2.1. |
@amigalemming fixed the version bound |
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.
Why can't this PR just be an upper version bound update on filepath
?
The OsString
type is re-exported from filepath
, so there's no need to include another dependency (in this case, os-string
).
@LaurentRDC it probably could, but then users of old I'm undecided whether I want to allow that option (and as it is, this PR wont: if you want I'll decide when GHC-9.10 is closer to release, when it's forced. Until then, indeed, things "seem" to work if you just relax bounds (e.g. with |
Use the new os-string package and filepath pacakage version >= 1.5.0.0 | ||
|
||
manual: False | ||
default: False |
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.
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.
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.
There's no way around it. It has to be an automatic flag.
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 know it has to be automatic, but can't it be default-true automatic?
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 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.
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.
@josephcsible if you are already passing --allow-newer
you can pass -c 'hashable +os-string'
as well.
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.
@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.
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.
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?
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 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
.
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.
The solution that was just merged in #293 doesn't cause this problem, so I'm happy with it.
@@ -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 |
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'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
.
@phadej : Would it be possible to release this now (or some other fix of the problem)? Evaluation of GHC 9.10 is hampered atm since 1000 projects depend on |
GHC 9.10 prerelease is out and I'm running into this in ghcup-metadata CI. |
Allows using filepath version >=1.5.0.0 and the new os-string package