Skip to content

Comments

support tar-0.7.0.0#11209

Merged
mergify[bot] merged 1 commit intohaskell:masterfrom
geekosaur:tar-0.7.0.0
Sep 10, 2025
Merged

support tar-0.7.0.0#11209
mergify[bot] merged 1 commit intohaskell:masterfrom
geekosaur:tar-0.7.0.0

Conversation

@geekosaur
Copy link
Collaborator

@geekosaur geekosaur commented Sep 6, 2025

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 •
edited
Loading

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

geekosaur commented Sep 6, 2025

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
cabal haddock failure: out of FDs due to tar #11131
Closed
@Bodigrim
Copy link
Collaborator

Bodigrim commented Sep 6, 2025 •
edited
Loading

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

cabal/cabal-install/src/Distribution/Client/Tar.hs

Lines 58 to 59 in 6915c20

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

geekosaur commented Sep 6, 2025

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 •
edited
Loading

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
View reviewed changes
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

geekosaur commented Sep 7, 2025

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

geekosaur commented Sep 7, 2025

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

geekosaur commented Sep 7, 2025

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
Bodigrim
Bodigrim reviewed Sep 7, 2025
View reviewed changes
@geekosaur
support tar-0.7.0.0
85c6c98 
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

geekosaur commented Sep 7, 2025

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

Bodigrim
Bodigrim approved these changes Sep 7, 2025
View reviewed changes
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

geekosaur commented Sep 8, 2025

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)

Mikolaj
Mikolaj approved these changes Sep 8, 2025
View reviewed changes
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!

This was referenced Sep 8, 2025
Closed
Closed
@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
@Mikolaj Mikolaj mentioned this pull request Sep 23, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Mikolaj Mikolaj Mikolaj approved these changes

@Bodigrim Bodigrim Bodigrim approved these changes

@ulysses4ever ulysses4ever ulysses4ever left review comments

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

cabal haddock failure: out of FDs due to tar

5 participants

@geekosaur @Bodigrim @hasufell @Mikolaj @ulysses4ever