Skip to content

Backport #10650: add ghc 9.12.1 to sdist check list #10865

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

Closed
wants to merge 2 commits into from

Conversation

mergify[bot]
Copy link
Contributor

@mergify mergify bot commented Mar 27, 2025

For 3.14 branch which ships with it.

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).

This is an automatic backport of pull request #10650 done by [Mergify](https://mergify.com).

@mergify mergify bot mentioned this pull request Mar 27, 2025
2 tasks
@mergify mergify bot added the backport label Mar 27, 2025
@Mikolaj Mikolaj added merge me Tell Mergify Bot to merge and removed backport labels Mar 27, 2025
@mergify mergify bot added the backport label Mar 27, 2025
@Mikolaj Mikolaj removed the merge me Tell Mergify Bot to merge label Mar 27, 2025
@ulysses4ever
Copy link
Collaborator

@geekosaur could you advise on what this sdist-check job failure mean and how to go about it? https://github.com/haskell/cabal/actions/runs/14106035233/job/39512705909?pr=10865

@geekosaur
Copy link
Collaborator

Looks like it's just missing an import (it's exported from cabal-install-solver/src/Distribution/Solver/Types/ProjectConfigPath.hs). But it's worrisome because it means check-sdist may have found a bug in cabal-install, or more to the point an API change (which is what it's supposed to be doing).

@geekosaur
Copy link
Collaborator

geekosaur commented Mar 27, 2025

The more I think about this, the less sense it makes. The sdist check is intended to verify that a newer cabal-install builds with the Cabal that ships with released ghcs. But this is an import issue between cabal-install and cabal-install-solver, and both are provided as sdists because they don't ship with ghc. So any import problem should also show on master.

@geekosaur
Copy link
Collaborator

Hm, right, I just checked the check-sdist job: it only makes an sdist for cabal-install, so it's validating against a cabal-install-solver from Hackage (the build tree has been removed for this check). So this probably means we have an API change in cabal-install-solver. I don't know if we care.

@Mikolaj
Copy link
Member

Mikolaj commented Mar 28, 2025

So what's the recommendation?

@geekosaur
Copy link
Collaborator

I don't know. Someone higher up the ladder than me needs to decide if an API change in cabal-install-solver is an issue, and if we decide not we need to update its bounds in cabal-install.cabal. (If we decide it is, we need to find the PR that made this change and possibly revert it.)

@geekosaur
Copy link
Collaborator

Oh, and the above won't make the sdist check succeed since we haven't released an updated cabal-install-solver yet (we release it along with cabal-install). I don't think there's a good way to make this use two sdists unless I create a project file specifically for the sdist check as a hack.

@geekosaur
Copy link
Collaborator

Mmm, actually I think I could get away with cabal install --lib --package-env=. to get the updated cabal-install-solver.

@Mikolaj
Copy link
Member

Mikolaj commented Mar 28, 2025

That all sounds terribly complicated for a point release. Let's drop the backport in that case, shall we?

@geekosaur
Copy link
Collaborator

We could drop it, but that would be sweeping the real problem under the rug.

@Mikolaj
Copy link
Member

Mikolaj commented Mar 28, 2025

Oh, why would it? I can't see this problem on master branch? Or, do you mean, we'd get the same problem on 3.16 branch, etc.?

@geekosaur
Copy link
Collaborator

No, I mean we would release 3.14.2 with an unexpected API change. We should decide what we want to do about that.

My current thinking is that cabal-install-solver is more or less internal, so an API change is actually okay. That said, I think Oleg uses it, so he might get a nasty surprise. (But that said, we release it with cabal-install, so just bumping the bound in cabal-install.cabal to ensure the correct one is used is probably good enough.)

@ulysses4ever ulysses4ever force-pushed the mergify/bp/3.14/pr-10650 branch from b4ba79a to 1c74699 Compare March 28, 2025 18:46
@geekosaur geekosaur force-pushed the mergify/bp/3.14/pr-10650 branch 6 times, most recently from 83ea33a to d24f731 Compare March 29, 2025 02:12
@geekosaur
Copy link
Collaborator

Is there a PR fixing addInternalBuildToolsFixed yet?

@geekosaur
Copy link
Collaborator

geekosaur commented Mar 29, 2025

Looking more closely (I was in a bit of a rush at the time), it looks like the revbump hasn't landed yet so it's still using the older Cabal.

@geekosaur geekosaur force-pushed the mergify/bp/3.14/pr-10650 branch from d24f731 to 624cf54 Compare March 29, 2025 04:02
@geekosaur
Copy link
Collaborator

It just landed, in fact, so I've rebased and will let this go until morning.

@geekosaur
Copy link
Collaborator

Argh! Apparently the solver is not considering things in the environment file??? I'll look into this tomorrow, it's past midnight here.

@ulysses4ever
Copy link
Collaborator

Make a project file instead of environment?

@geekosaur
Copy link
Collaborator

Sadly, the project file method would cause those versions to be tried first as far as I can tell, which would defeat the purpose.

For 3.14 branch which ships with it.

(cherry picked from commit bddd03f)
@geekosaur geekosaur force-pushed the mergify/bp/3.14/pr-10650 branch from 624cf54 to c5c92f7 Compare March 29, 2025 20:38
It sometimes happens that we have API updates that don't violate
PVP (e.g. adding things). In those cases, we need the newer version
available and cabal-install's dependencies must be updated to
require it instead of the bootlib.

We stash these in an environment file, since that's the most
convenient way to get them into cabal-install's build environment.
@geekosaur geekosaur force-pushed the mergify/bp/3.14/pr-10650 branch from c5c92f7 to 9aef702 Compare March 29, 2025 20:49
@geekosaur
Copy link
Collaborator

@grayjay, do you know if the solver considers things in GHC environment files, or only from Hackage and other repos? Do I need to set up a local+noindex repo to do this?

@grayjay
Copy link
Collaborator

grayjay commented Mar 31, 2025

@geekosaur I'm not very familiar with GHC environment files, but I only know of cabal writing them (with cabal install --lib), not reading them. I'm not sure how packages from the environment file would interact with the store. Why wouldn't a project file containing only cabal-install and cabal-install-solver work?

@geekosaur
Copy link
Collaborator

The confusing thing here is, they should already be in the store; the environment file just exposes them to ghc. Where they aren't is in any declared repo, and I'm worried that the solver only looks at that (see also #10504 (comment)).

@ulysses4ever
Copy link
Collaborator

Let me see if I understand the problem. This workflow is meant to check cases where we want to release cabal-install x.y.z where y > 1. We need the solver to both

  • have Cabal x.y'.z where y' <= y (the = is crucial) available
  • prefer Cabal x.{y-1}.z if it's allowed by constraints in cabal-install-x.y.z. or just the oldest Cabal allowed?

Is this anywhere close to the rules we want to implement? I feel like I've never understood the spec for this workflow, so I struggle to understand what rules we are trying to implement. The workflow is called "sdist check" but what is our mission actually? Is it that cabal-install from branch X (only release branches or master too?) sdist/installs? Are we trying to "check API changes" for Cabal(-syntax)? I hope not because it sounds like a fairly independent task.

Back to the rules. Can the second rule be implemented with --prefer-oldest? Are we only having trouble with the first rule? The first rule, you're saying, can't be implemented with adding a local Cabal-x.y.z (via a project file), but doesn't --prefer-oldest save the day? I seem to remember that local packages are always preferred, and the answer to this is that --prefer-oldest can't help, but I'd double-check.

If --prefer-oldest and local packages don't work together as we'd like them to, do we have to apply more extreme measures and construct our own package database, or run our own pre-populated Hackage instance, or something like that? This is probably not impossible. But it'd be great to understand the spec here...

@geekosaur
Copy link
Collaborator

geekosaur commented Mar 31, 2025

Okay, so the root of the problem check-sdist is intended to solve is #10385. I thought there was another one for 3.10.3, but I don't see it on a quick check. The 3.10.3 one is actually what caused me to develop check-sdist.

So the idea is to try to ensure in CI that cabal-install will install with the Cabal it declares, which because cabal-install defaults to ^>=3.x.0.0 and the solver prefers bootlibs over other versions usually means the version included with ghc. (This also means --prefer-oldest won't work, since the solver's bootlib preference 99% of the time means it's doing --prefer-oldest anyway for Cabal.)

The problem we're running into here is that a (valid, apparently) non-API-breaking change is being depended on by cabal-install, which as designed meant check-sdist failed because it won't build with the bootlib. So we bumped cabal-install's dependency on Cabal, but now we have the problem that it's not released yet so it can't retrieve it from Hackage.

I can't use the one from the build tree because if I use a project file to expose it, it will effectively lock us to the version in the build tree (vendoring overrides the solver's choice, by design since that's the whole point of vendoring), which gets us back to the original problem that we can't tell if users who don't have the full build tree will be able to build cabal-install.

As far as I can tell, the solver doesn't consider versions already in the store or exposed via --package-env, it only considers versions available to it from repositories and possibly the global package-db because of its bootlib preference. So I will need to set up a local+noindex repo containing the updated dependencies and a minimal non-vendoring project file, without override on the repo so its contents will only be considered if the versions from Hackage don't match cabal-install's build-depends (thus ensuring this doesn't again get us back to not seeing if it will work for ordinary users).

@Mikolaj
Copy link
Member

Mikolaj commented Apr 1, 2025

Copying from Matrix:

geekosaur@geekosaur:matrix.org says:

re #10865, I need to reimplement it and it should really be done on master and backported. for now I think the way forward for 3.14 is to remove it and its skip job (if that's still there)

@Mikolaj
Copy link
Member

Mikolaj commented Apr 1, 2025

@geekosaur: what "skip job" do you mean? Is it a skipped test? If so, can we just let it be?

@geekosaur
Copy link
Collaborator

I mean #10870 and its parent, they're part of the docs-only stuff that broke Mergify.

@Mikolaj
Copy link
Member

Mikolaj commented Apr 1, 2025

Oh, I see. IIRC #10870 and it counterpart on master already reverted the "skip job" mechanism, so maybe 3.14 is fine? If not, what remains to be done?

@Mikolaj Mikolaj closed this Apr 1, 2025
@geekosaur
Copy link
Collaborator

Again, the sdist check needs to be removed from this branch and my work to rewrite it needs to be continued on master. Unless it's decided that simply leaving 9.12.1 off the checklist for now is good enough, in which case we do close this PR and I still have to make a new version on master to fix the sdist check.

@Mikolaj
Copy link
Member

Mikolaj commented Apr 1, 2025

Right, now I understand what is missing: 9.12.1 support. For 3.14.2, let's leave it as is. If there's any other minor release of 3.14, we can do things properly as part of backporting for the release, then also using your new work on master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants