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 GHC-9.6 (rebased) #1127

Closed
wants to merge 16 commits into from
Closed

Support GHC-9.6 (rebased) #1127

wants to merge 16 commits into from

Conversation

hseg
Copy link

@hseg hseg commented Sep 11, 2024

Rebased version of #997 onto master.

Notable edits:

Closes: #979, #997

@hseg
Copy link
Author

hseg commented Sep 11, 2024

Commentary on CI failures:

  • It seems .github/release.yaml and .github/workflows/shimgen.yaml should be upgraded away from upload-artifact@v2? https://github.blog/changelog/2024-02-13-deprecation-notice-v1-and-v2-of-the-artifact-actions/ I don't know Github workflows well, do you know how to update that?
  • I'm confused by the failures of cross.yaml:build and release.yaml:build-freebsd -- they complain about receiving a tar ==0.6.0.0 constraint, but git grep 'tar.*0.6.0.0 only yields the constraints tar ^>= 0.6.0.0 from ghcup.cabal

@hseg
Copy link
Author

hseg commented Sep 11, 2024

Ah wait, perhaps the remaining reference to the pinned commit of https://github.com/haskell/tar in cabal.project.release should be removed?

@hseg
Copy link
Author

hseg commented Sep 11, 2024

(note that the reference to a pinned commit in uri-bytestring is the as-yet-unmerged Soostone/uri-bytestring#64, so it can't be removed yet)

hasufell and others added 7 commits September 11, 2024 15:55
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
@hseg
Copy link
Author

hseg commented Sep 11, 2024

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.

@hseg
Copy link
Author

hseg commented Sep 11, 2024

Unfortunately, this still doesn't fix the weird tar ==0.6.0.0 bug -- any ideas where cabal is picking that idea up? EDIT: wait, the logs say
HEAD is now at d94a988 Codec.Archive.Tar.Read: refactor getEntry
-- is there still a reference to the commit somewhere?
EDIT2: Can't find any such references, very weird

@hseg
Copy link
Author

hseg commented Sep 11, 2024

BTW, it is unclear to me why these lines were removed from cabal.project.release in the original branch:

elif os(mingw32)
  constraints: zlib +bundled-c-zlib,
               lzma +static,
               text -simdutf,
               vty-windows >=0.2.0.2
  if impl(ghc >= 9.4)
    constraints: language-c >= 0.9.3

(Now, tbf they do occur in cabal.project, but mingw32 is the only platform that was previously duplicated there, and it is unclear why it was deduplicated in favor of cabal.project rather than cabal.project.release like linux, darwin and freebsd)

@hseg
Copy link
Author

hseg commented Sep 11, 2024

Wait, shouldn't I have dropped references to cabal-{install-parsers,plan}, given the migration away from it?

@hseg
Copy link
Author

hseg commented Sep 11, 2024

Oh wait, missed the existence of scripts/dev/refreeze.sh.
Should I be using that, instead? I don't have access to a windows box, so I can't create the windows freezefiles.

Cf fix-hashable-mess, presumably this should be added here as well
@hasufell
Copy link
Member

I can handle the windows files. just update the unix ones

@hseg
Copy link
Author

hseg commented Sep 11, 2024

Missed one more migration: streamly -> conduit in cabal.project.release

@hseg
Copy link
Author

hseg commented Sep 11, 2024

Was 411ac8d supposed to affect cabal.project.release as well?
It guarded ghcup +tar behind an impl(ghc < 9.0)

Based on the behaviour of the other cabal.*.project files, introduced in
411ac8d. I'm presuming it was forgotten
@@ -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"]
Copy link
Member

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?

Copy link
Author

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
Copy link
Member

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?

Copy link
Author

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)

Copy link
Author

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

Copy link
Member

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


optimization: 2

package ghcup
flags: +tui -tar
if impl(ghc < 9.0)
Copy link
Member

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.

Copy link
Author

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)

Copy link
Author

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?

package ghcup
flags: +tui +tar

constraints: http-io-streams -brotli,
Copy link
Member

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?

Copy link
Author

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

Copy link
Author

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
@hasufell
Copy link
Member

Can you revert the reformatting of the cabal files? Let's please keep the merge conflicts to a minimum.

@hasufell
Copy link
Member

i squashed and rebased and set you as co-author in the commit, thanks: #1132

@hasufell hasufell closed this Sep 28, 2024
@hseg
Copy link
Author

hseg commented Sep 28, 2024 via email

@hseg hseg deleted the rebased-ghc-9.6 branch September 30, 2024 10:44
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.

GHC-9.6 support
3 participants