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

Depend on tar-0.6.3.0 #10120

Merged
merged 2 commits into from
Jun 18, 2024
Merged

Depend on tar-0.6.3.0 #10120

merged 2 commits into from
Jun 18, 2024

Conversation

mpickering
Copy link
Collaborator

tar-0.6.3.0 has much improved performance of deserialising .tar index
which has significant ramifications for the start-up time of
cabal-install.

See #10110

@ffaf1
Copy link
Collaborator

ffaf1 commented Jun 17, 2024

I wonder if this compat module can be axed then.

{- FOURMOLU_DISABLE -}
module Distribution.Client.Compat.Tar
( extractTarGzFile
#if MIN_VERSION_tar(0,6,0)
, Tar.Entry
, Tar.Entries
, Tar.GenEntries (..)
, Tar.GenEntryContent (..)
, Tar.entryContent
#else
, Tar.Entries (..)
, Tar.Entry (..)
, Tar.EntryContent (..)
#endif
) where

I also wonder: are we introducing some indirect constraint on base?

@ulysses4ever
Copy link
Collaborator

It's great to hear about tar advancements. Reacting to these with bumping the lower bound seems too harsh to me.

@mpickering
Copy link
Collaborator Author

@ffaf1 A good question, tar builds with base >= 4.11 which goes back to 8.4.4 at least.

@ulysses4ever It's up to the maintainers what they wish to do, but older versions of tar have a non-performant implementation of deserialise so if you build cabal-install against them then you will suffer slower start-up times.

I will at least bump the index state for release and bootstrap jobs (which will then pick tar-0.6.3)

@ulysses4ever
Copy link
Collaborator

older versions of tar have a non-performant implementation of deserialise so if you build cabal-install against them then you will suffer slower start-up times.

I understand. I don't see why you want to disallow that. If your plan allows a newer version of tar, the solver will pick it up anyway. But otherwise, with this change, you just get stuck.

Updating the index state makes sense, thank you.

@mpickering
Copy link
Collaborator Author

I suppose I see it in the same way as if a previous version of the package had a serious bug, you wouldn't want to allow your users to build against that package. I'll remove the tight lower bound once CI passes.

tar-0.6.3.0 has much improved performance of deserialising .tar index
which has significant ramifications for the start-up time of
cabal-install.

See haskell#10110
Using the updated version of this library improves performance of
cabal-install so it's worthwhile to make sure the bootstrap plans use
this version.
@mpickering
Copy link
Collaborator Author

I have removed the more aggressive lower bound on cabal-install now (which doesn't seem like the best thing to do in my opinion).

So can someone please approve the remaining changes which bumps the index state and regenerates the bootstrap plans to include tar-0.6.3.0.

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.

LGTM

@Mikolaj
Copy link
Member

Mikolaj commented Jun 18, 2024

If nobody objects, let's expedite this PR and the backport.

@ffaf1
Copy link
Collaborator

ffaf1 commented Jun 18, 2024

What about the Compat module @mpickering? Is it still needed?

@Mikolaj
Copy link
Member

Mikolaj commented Jun 18, 2024

@ffaf1: we haven't bumped tar beyond (0,6,0), after all, so it seems it is needed? I guess we could have made such a modest bump, but we are in a hurry this time.

I haven't heard objections, so I'm force-merging.

@Mikolaj Mikolaj added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Jun 18, 2024
@mergify mergify bot merged commit 1c00507 into haskell:master Jun 18, 2024
52 checks passed
@Mikolaj
Copy link
Member

Mikolaj commented Jun 18, 2024

@mergify backport 3.12

Copy link
Contributor

mergify bot commented Jun 18, 2024

backport 3.12

✅ Backports have been created

@Mikolaj
Copy link
Member

Mikolaj commented Jun 18, 2024

Merged. @mpickering: thank you again.

And we have conflicts in the backport. :(

mergify bot added a commit that referenced this pull request Jun 18, 2024
* Bump index state to allow tar-0.6.3.0

tar-0.6.3.0 has much improved performance of deserialising .tar index
which has significant ramifications for the start-up time of
cabal-install.

See #10110

(cherry picked from commit 827256a)

* Update bootstrap plans to include tar-0.6.3

Using the updated version of this library improves performance of
cabal-install so it's worthwhile to make sure the bootstrap plans use
this version.

(cherry picked from commit 4d787e1)

# Conflicts:
#	bootstrap/linux-8.10.7.json
#	bootstrap/linux-9.0.2.json
#	bootstrap/linux-9.2.8.json
#	bootstrap/linux-9.4.8.json
#	bootstrap/linux-9.6.4.json
#	bootstrap/linux-9.8.2.json

* fixup! Update bootstrap plans to include tar-0.6.3

---------

Co-authored-by: Matthew Pickering <matthewtpickering@gmail.com>
Co-authored-by: brandon s allbery kf8nh <allbery.b@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants