Skip to content

Conversation

@geekosaur
Copy link
Collaborator

The changelog says the only significant change is to GenEntries, which we don't appear to use (local build would have gotten a type error).


Template B: This PR does not modify behaviour or interface

E.g. the PR only touches documentation or tests, does refactorings, etc.

Include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Is this a PR that fixes CI? If so, it will need to be backported to older cabal release branches (ask maintainers for directions).

@geekosaur
Copy link
Collaborator Author

geekosaur commented Sep 6, 2025

You know, this is skipping FreeBSD whereas my fork is trying to run it. Is there a flipped conditional somewhere? /cc: @hasufell

@geekosaur
Copy link
Collaborator Author

Looks like to get CI to do it requires --allow-newer:tar to satisfy dependencies' constraints.

@Bodigrim Bodigrim linked an issue Sep 6, 2025 that may be closed by this pull request
@Bodigrim
Copy link
Collaborator

Bodigrim commented Sep 6, 2025

The reason for tar-0.7.0.0 release was to fix #11131, so presumably we want to update

createTarGzFile tar base dir =
BS.writeFile tar . GZip.compress . Tar.write =<< Tar.pack base [dir]

to BS.writeFile tar . GZip.compress =<< Tar.write' =<< Tar.pack' base [dir] when tar >= 0.7. It's not strictly necessary to do in this PR (one can argue that simply allowing a new tar is already a step forward), but I think it would make sense to combine.

@geekosaur
Copy link
Collaborator Author

I thought I'd remembered something along those lines, so I went looking at the changelog but it didn't tell me anything useful about that (I suppose it's not an API change but a per-consumer choice?).

@Bodigrim
Copy link
Collaborator

Bodigrim commented Sep 7, 2025

Yeah, I should have probably highlighted it in the changelog explicitly, sorry for this.

@geekosaur
Copy link
Collaborator Author

geekosaur commented Sep 7, 2025

Meanwhile our backward compatibility requirements mean hiding this in Distribution.Client.Compat.Tar, then using CI to test with the old version (one of our dependencies seems to be excluding 0.7.0.0) and a triggered run with CONSTRAINTS/ALLOW_NEWER to test the new. Or local testing like I did before pushing this.

@Bodigrim
Copy link
Collaborator

Bodigrim commented Sep 7, 2025

one of our dependencies seems to be excluding 0.7.0.0

I think it's hackage-security, should be easy to fix.

@hasufell
Copy link
Member

hasufell commented Sep 7, 2025

You know, this is skipping FreeBSD whereas my fork is trying to run it. Is there a flipped conditional somewhere? /cc: @hasufell

This PR does not run the release pipeline. It's skipped.

Your fork runs the release pipeline scheduled only and skips FreeBSD: https://github.com/geekosaur/cabal/actions/runs/17522152727

ulysses4ever
ulysses4ever previously approved these changes Sep 7, 2025
Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

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

Thanks!

@geekosaur
Copy link
Collaborator Author

Unlabeling and dismissing review until I have time to work on the updated call convention this afternoon. That's what will really need to be reviewed.

@geekosaur geekosaur dismissed ulysses4ever’s stale review September 7, 2025 13:44

change is incomplete, and the next step is the one that really needs to be reviewed

@geekosaur
Copy link
Collaborator Author

I think it's hackage-security, should be easy to fix.

Well, except that getting a change released so we can use it can incur a delay, and conceivably it will require the same kind of modification.

@geekosaur
Copy link
Collaborator Author

BTW is there some particular reason why Distribution.Client.Compat.Tar has a #if with an empty true branch instead of #if !…?

@geekosaur geekosaur force-pushed the tar-0.7.0.0 branch 2 times, most recently from eb76dc8 to 08d50a2 Compare September 7, 2025 19:30
Use the new functionality (which didn't make the changelog) when
available; see haskell#11131. This moves `createTarGzFile` to
`Distribution.Client.Compat.Tar` with a re-export.
@geekosaur
Copy link
Collaborator Author

This is what I get for having to get up 2 hours earlier than usual, sigh.

Copy link
Collaborator

@Bodigrim Bodigrim left a comment

Choose a reason for hiding this comment

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

I tested that this patch is compatible with both old and new tar by running

cabal build cabal-install -c 'tar<0.7'

and

cabal build cabal-install -c 'tar>=0.7' --allow-newer='hackage-security:tar'

@geekosaur
Copy link
Collaborator Author

Hrm. Apparently I can't run a triggered workflow on a PR? (after having to dig into the workflow file to see how to specify the allow-newer/constraints correctly; I'm making a note to add that to the maintainers readme in #10503)

Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

Thank you!

@geekosaur geekosaur added the merge me Tell Mergify Bot to merge label Sep 8, 2025
@mergify mergify bot added ready and waiting Mergify is waiting out the cooldown period merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days labels Sep 8, 2025
@mergify mergify bot merged commit 30f75ae into haskell:master Sep 10, 2025
208 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge ready and waiting Mergify is waiting out the cooldown period

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cabal haddock failure: out of FDs due to tar

5 participants