-
Notifications
You must be signed in to change notification settings - Fork 90
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 GHC-9.6 (rebased) #1127
Conversation
Commentary on CI failures:
|
Ah wait, perhaps the remaining reference to the pinned commit of https://github.com/haskell/tar in |
(note that the reference to a pinned commit in |
Original branch had freezes undone by 154a866 (hereafter: fix-hashable-mess), erred on the side of going with its newer situation. Doubts regarding my choices: - The `hspec` major versions have increased since to 2.11.* (over 2.10.* referred to there) See below where I kept the old version - `os-string` had a "bad" version 2.0.2.1 that was bumped everywhere, except in ghc{928,948}.Win32 where it had been at 2.0.2 -- it is unclear if that should've been bumped as well. Erred on the side of keeping those cases where the original branch had added `os-string ==2.0.3` given that that is the "good" version - Some dependencies were only touched in some freezefiles by fix-hashable-mess, so I kept the cases where they weren't touched around: - `hspec`: ghc*.Win32, excluding ghc965.Win32 - `tasty` and friends: ghc*.Win32, excluding ghc965.Win32 - `setenv`: ghc*.Win32, excluding ghc965.Win32 (I didn't know what the correct versions to use for ghc965.Win32 were, so I didn't try guessing)
Given the presence of cabal.ghc965.*, I assume that's the correct version to target
644f9ab
to
48e6c87
Compare
Hrm. I should probably not be force-pushing commits anymore now you're reviewing them, right? Sorry for that, will stop doing so if there are more modifications to be made. At least Github is smart enough to give you access to the diff between the two branch states. |
Unfortunately, this still doesn't fix the weird |
BTW, it is unclear to me why these lines were removed from
(Now, tbf they do occur in |
Wait, shouldn't I have dropped references to |
Oh wait, missed the existence of |
Cf fix-hashable-mess, presumably this should be added here as well
I can handle the windows files. just update the unix ones |
Missed one more migration: |
Was 411ac8d supposed to affect |
This aids in diffing the two
Based on the behaviour of the other cabal.*.project files, introduced in 411ac8d. I'm presuming it was forgotten
.github/workflows/cabal.project.yaml
Outdated
@@ -20,12 +20,14 @@ jobs: | |||
fail-fast: false | |||
matrix: | |||
os: [macOS-12, windows-latest, ubuntu-latest] | |||
ghc: ["8.10.7", "9.0.2", "9.2.8", "9.4.8"] | |||
ghc: ["8.10.7", "9.0.2", "9.2.8", "9.4.8", "9.6.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.
Why not 9.6.6?
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.
Old branch was on 9.6.5, wasn't sure if you'd appreciate me bumping it.
, variant ^>=1.0 | ||
, megaparsec >=8.0.0 && <9.3 | ||
, mtl ^>=2.2 | ||
, aeson >=1.4 |
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.
what's the point of reformatting here?
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 noticed the spacing had ended up inconsistent after the last couple of edits there, and just blasted it with cabal-fmt
. I can either drop this reformatting, take the less-noisy option of just realigning the outliers, or reformat it with gild to avoid such issues in the future. Your choice. (Also, please let me know if you'd prefer me just dropping/squashing the reformatting commit or if you'd like me to revert the reformatting on top of the current tip)
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.
Just to make things concrete, I pushed a branch to my fork with the gild
-formatted version of the cabal files. Let me know if it's in any way useful to you
https://github.com/hseg/ghcup-hs/tree/reformat-cabal
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 reformatting makes it painful to fix merge conflicts
cabal.project.release
Outdated
|
||
optimization: 2 | ||
|
||
package ghcup | ||
flags: +tui -tar | ||
if impl(ghc < 9.0) |
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.
This is not correct. The release project file always wants to build with -tar
. The tar flag is only there to circumvent complicated errors during development, which sometimes happens due to libarchive.
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.
Ah, OK. I'll drop a note in a commit reverting this, let me know if you want me to squash them together (so the resulting commit will just add a comment noting this discrepancy vs the other cabal.project files)
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.
Also, is this why the mingw32
flags are present in cabal.project
(and will be imported in cabal.project.release
, whereas the linux
, darwin
, and freebsd
flags are only present in cabal.project.release
? Windows causes more trouble that needs to be caught at dev time, but the others are mostly just optimizations for release time?
cabal.project.release
Outdated
package ghcup | ||
flags: +tui +tar | ||
|
||
constraints: http-io-streams -brotli, |
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 add these when you already did import: cabal.project
?
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.
True, though by that logic, shouldn't cabal.project.release
be simplified to:
import: cabal.project
optimization: 2
if os(linux)
if arch(x86_64) || arch(i386)
package *
ghc-options: -split-sections -optl-static
elif os(darwin)
constraints: zlib +bundled-c-zlib,
lzma +static
elif os(freebsd)
constraints: zlib +bundled-c-zlib,
zip +disable-zstd
package *
ghc-options: -split-sections
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.
BTW, these constraints already existed at the bottom of the file -- I just moved them to the same place they were in the other cabal.project
files
As explained by @hasufell > This is not correct. The release project file always wants to build > with -tar. The tar flag is only there to circumvent complicated errors > during development, which sometimes happens due to libarchive. Add a note citing the above to the file to prevent future mixups
Given that we import most of the configuration from cabal.project, there's no reason to duplicate it in cabal.project.release
Can you revert the reformatting of the cabal files? Let's please keep the merge conflicts to a minimum. |
i squashed and rebased and set you as co-author in the commit, thanks: #1132 |
Thanks, have had much less free time in the past week, so I couldn't get back to you.
El 28 de septiembre de 2024 16:26:45 GMT+03:00, Julian Ospald ***@***.***> escribió:
…i squashed and rebased and set you as co-author in the commit, thanks: #1132
--
Reply to this email directly or view it on GitHub:
#1127 (comment)
You are receiving this because you authored the thread.
Message ID: ***@***.***>
|
Rebased version of #997 onto
master
.Notable edits:
app/ghcup/Main
:Cabal.Plan
->PlanJson
->GHCup.PlanJson
lib-opt/GHCup/OptParse/Common.hs
: Reflect extraction of some parts tolib/GHCup/Utils/Parsers.hs
app/ghcup/BrickMain.hs
5b41f79 for doubts regarding my choices.
Similarly, picked up some version bumps on the way (the unix freeze files
are more affected by this).
I'm least confident about this edit, it might require polishing
haskus-*
->variant
cabal.*.project
:tar
repository that had been added is nowreleased, I dropped the
source-repository-package
directivehashable
needing-arch-native
cabal-{install-parsers,plan}
cabal.project.release
:tar
commit, like in the othercabal.*.project
filescabal-{install-parsers,plan}
streamly
and cocabal.*.project
filesghcup +tar
logic found in othercabal.*.project
filesghcup.cabal
:cabal-{install-parsers,plan}
in favor ofCabal-syntax
andcreation of
GHCup.{CabalConfig,PlanJson,Utils.Parsers}
parsec
,utf8-string
haskus-*
->variant
optics
moving intoapp-common-depends
ghcup-tui
includes:
stack.yaml
:haskus-*
->variant
tar-0.6.3.0
instead of commit, pick upghcup +tar
flagmintty +win32-2-13-1
flagtested-with
toghc-9.6.5
based on the existence ofcabal.ghc965.*
.orig
files after it was confirmed they were unintentionally added.Closes: #979, #997