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

Avoid partial Data.List.last in autogenerated Paths_<pkg>.hs #10432

Merged
merged 1 commit into from
Oct 9, 2024
Merged

Conversation

mpilgrem
Copy link
Collaborator

@mpilgrem mpilgrem commented Oct 6, 2024

Motivation (in part):

I have reasoned that this minor refactoring is correct, but I am not sure how to test it. EDIT: What I have done is:

  • unit tested lastChar in GHCi; and

  • tested that a separate copy of the module does compile.

  • 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). N/A

mpilgrem added a commit that referenced this pull request Oct 6, 2024
@Bodigrim
Copy link
Collaborator

Bodigrim commented Oct 6, 2024

Would it work if Cabal is driving a very old GHC (or non-GHC Haskell implementation) which does not yet have Data.List.NonEmpty in base? I think it's better to inline

lastM :: [a] -> Maybe a 
lastM = foldr (\x -> Just . fromMaybe x) Nothing

(The existence of fromMaybe is guaranteed by Haskell Report 2010)

@geekosaur
Copy link
Collaborator

I did think about that, but I also think nobody bothers to care about non-GHC any more (I think Data.List.NonEmpty is in all the ghcs we support). That ship sailed long ago, sadly.

@mpilgrem
Copy link
Collaborator Author

mpilgrem commented Oct 6, 2024

The stated GHC support window for Cabal is five years, which means GHC 8.8.2 (released 16 January 2020) on. GHC 8.8.2 comes with base-4.13.0.0, which exposes Data.List.NonEmpty. Data.List.NonEmpty joined base in 4.9.0.0 (GHC 8.0.1, released 21 May 2016).

The Cabal project does not identify any support for non-GHC compilers of Haskell.

@Bodigrim
Copy link
Collaborator

Bodigrim commented Oct 6, 2024

The stated GHC support window for Cabal is five years, which means GHC 8.8.2 (released 16 January 2020) on. GHC 8.8.2 comes with base-4.13.0.0, which exposes Data.List.NonEmpty.

My understanding always was that this is a support window for GHC to compile Cabal sources, not which GHC can be driven by Cabal. Changing Paths_<pkg>.hs affects the latter. Could this be clarified in CONTRIBUTING.md?

IIRC Cabal 3.12 was happily able to drive GHC 7.0 and such.

@geekosaur
Copy link
Collaborator

We stopped being able to test anything before 8.0 because ghcup's only 7-series for Ubuntu segfaults during install.

@mpilgrem
Copy link
Collaborator Author

mpilgrem commented Oct 6, 2024

@Bodigrim, you are correct - I misunderstood CONTRIBUTING.md (which is clear, now you point it out).

As we know that dir@(c:cs) is non-empty, we could use:

lastChar c [] = c
lastChar _ (c:cs) = lastChar c cs

@mpilgrem
Copy link
Collaborator Author

mpilgrem commented Oct 6, 2024

@geekosaur, second attempt at this submitted, hopefully meeting @Bodigrim's observation.

The 'required' CI 'Quick jobs / Meta checks' is failing at step 'Check that diff is clean'. I do not know what that is testing, or why it is failing.

@geekosaur
Copy link
Collaborator

I saw that, and I don't know either (I can't figure out how a commit pushed to GitHub could fail that check??).

@geekosaur
Copy link
Collaborator

And I just checked out your branch locally, and it's clean here. (Both the diff against master (and HEAD^) and the git diff-files -p check that's failing.)

@Mikolaj, @Kleidukos do either of you know what's going on here?

@ulysses4ever
Copy link
Collaborator

ulysses4ever commented Oct 7, 2024

@mpilgrem you'll have to update https://github.com/haskell/cabal/blob/master/templates/Paths_pkg.template.hs according to your changes to Z.hs. See, for example, the previous instance of such a coordinated update: cc8d5df

@Mikolaj
Copy link
Member

Mikolaj commented Oct 7, 2024

Would it work if Cabal is driving a very old GHC (or non-GHC Haskell implementation) which does not yet have Data.List.NonEmpty in base?

Thinking aloud, if it's driving a compiler, not compiling with it, NonEmpty should work fine regardless whether it's supported by the compiler. What would matter if some compiler options were missing, but that's very unrelated.

Also thinking aloud, tests would be great, but there's no way to test this, except for adding a STAN run to CI or checking for a new GHC warnings to the same effect (though I don't remember if the new warnings complain about last).

@mpilgrem mpilgrem force-pushed the fixLast branch 2 times, most recently from 4186325 to e4e3f60 Compare October 7, 2024 09:33
@mpilgrem
Copy link
Collaborator Author

mpilgrem commented Oct 7, 2024

@ulysses4ever, many thanks. This (final) version now passes the CI.

@mpilgrem mpilgrem added the merge me Tell Mergify Bot to merge label Oct 7, 2024
@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 Oct 7, 2024
@mergify mergify bot merged commit 1b5f97e into master Oct 9, 2024
43 checks passed
@mergify mergify bot deleted the fixLast branch October 9, 2024 21:22
@geekosaur
Copy link
Collaborator

Sigh. Still got merged prematurely.

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.

5 participants