componentAvailableTargetStatus: impossible; cname=test #8719
Comments
|
The culprit commit is b547ead The diff --git a/cabal-install/src/Distribution/Client/ProjectPlanning.hs b/cabal-install/src/Distribution/Client/ProjectPlanning.hs
index 4ec141037..4dbe4d74b 100644
--- a/cabal-install/src/Distribution/Client/ProjectPlanning.hs
+++ b/cabal-install/src/Distribution/Client/ProjectPlanning.hs
@@ -2143,7 +2143,7 @@ isInLocal _extraPackages (SpecificSourcePackage pkg) = case srcpkgSource pkg of
-- as these flags would apply to local packages, but the sdist would
-- erroneously not get categorized as a local package, so the flags would be
-- ignored and produce a package with an unchanged hash.
- LocalTarballPackage _ -> Just (packageId pkg)
+ -- LocalTarballPackage _ -> Just (packageId pkg)
-- TODO: the docs say ‘extra-packages’ is implemented in cabal project
-- files. We can fix that here by checking that the version range matches.
--RemoteTarballPackage _ -> _Seems to fix the issue. I was right telling @Mikolaj in private that it's probably because someone consused local as in "on my machine" with local is in "project package". That is unfortunately illogical/ambiguous naming in |
|
another code path without regression test (and this seems to be an important one not too difficult to add) 😓 |
|
A general instance of not having regression tests for |
On my system, all tests passed 100%, and this also seemed to fix the error in the sceanior mentioned, but this needs to be completed. (I guess also double check the surrounding code to make sure it's a good approach.)
|
Thanks for the report and simple scenario I could reproduce. I authored that commit along with the test that follows it. It seems to me it unmasks another error. Sorry that's given you issues. To me it seems there are disagreeing perspectives on what ‘local’ means or what's better. I suppose the majority can decide, or policy can decide (e.g. 2 reviewers for this distribution), or in principle anyway forks could be made, if I'm thinking about this well. Here's what I'm getting from the docs (source By my interpretation of the these docs, it seems it'd be required to me according to that approach. Here's what I'm getting from the understanding that a package in a project is not local if it's an a tarball, but if it's unpacked then that package is local: You say: If the spec that only calls unpacked intra-project packages local wins, then actually ‘--enable-tests’ should only apply presumably to building the referenced packages, although with ‘all’ specified to build all components of all packages, then I'd still expect ‘--enable-tests’ to apply to both the package referred to by ‘.’ and the package referred to by ‘splitmix-0.1.0.4.tar.gz’. (Perhaps the interface could be updated to accommodate both interpretations, so that one build config flag affects all project-local packages, another affects globally all packages being built, another affects only project-local unpacked packages, and perhaps another affects only packages that meet the specified patterns.) I think if it were put to a vote, I'd probably be in favor of the docs at least, but better (with the cost of it being more work) might be to support more use cases such as specifying the configuration for any package built, or to restrict the configuration to only specified packages. (If it is further developed, while that would probably require work, D.C.ProjectPlanning.Types does seem to note an in-code task that could optionally be worked on:) Anyway, while I may disagree with that particular point, or at least on which perspective I prefer, it does look like there's a real error that's being brought up, which I can reproduce in your scenario. The surrounding code looks like this: I took a look at it, and it looks like my older fix unmasks this ‘error’ branch, since the ‘TargetNotLocal’ branch was being hit too often - I think so, anyway. I wrote a quick draft of what seemed to be to be likely a good approach to fix this issue and pushed it to my ‘wip-target-not-available’ branch with the following diff: I'd probably like to double check the code around it to verify it seems to make sense, as that particular part of the code is one I haven't worked through as much as some of the other parts, and the various TODO messages and details would need to be filled/worked out. But it did seem to fix the error in your scenario. If nobody beats me to it, I'll likely have time available to set aside for this next weekend (last weekend unfortunately other things prevented me from being able to contribute to Cabal that weekend). Now if you're in a rush to get the next release out before this is done and would rather revert the fix at least in the meantime, trading one error for another, I think that's fine and don't want to get in the way of slowing it down. But given that it's OSS and I already submitted the contributions, I'm not sure it'd much matter anyway. Or if you want me to submit a GitHub approval to get it reverted, I guess I can do that too. I could just be one part of the majority, outside my own fork. |
|
(And one more note: IIRC, WRT the original fix, ‘cabal-install v2-install’ looked like it was producing its own sdist from the unpacked package and the build flags were getting dropped, but I might need to double check that.) (Also tagging people likely to be interested in this update: @Mikolaj @gbaz (I didn't notice this discussion on this merged PR until I was pinged, so maybe I could also practice utilizing this GitHub feature better.)) |
|
@bairyn: thank you very much for having a look again. What do you think about reverting half of the PR in #8744 (not reverting the test, though) so that you have ample time to revisit the changes, split them up and we can review them slowly one by one and with better understanding (I apparently lacked sufficient understanding when reviewing last time)? I think there is no point to rush and a revert would take the pressure of from us. |
|
Okay. |
|
OK, we have a clean slate, we can start adding things back. @bairyn: please feel free to re-add stuff and please be mindful some us (e.g., me) require the innovation to be served in very small bites to review them effectively. It would be great if we could also add tests to each of these features, because this old PR was a very important one, touching/improving very important areas, but the high value comes with high risk and tests mitigate that. @gbaz: I remember you were especially keen to bring one of the features from the old PR back, perhaps even into 3.10, if we manage to in time. Which one was that? |
|
Could you kindly check if the bug if perhaps gone in this version with some fixes: https://gitlab.haskell.org/haskell/cabal/-/pipelines/63307 The commandline to get the Linux version via ghcup is |
|
should be resolved by #8744 |
As I wrote on the other ticket, the important thing from the reverted pr was "teaching cabal to use the same "way" (i.e. dynamic or static) to build the setup script as it uses to build the package itself". |
When a project uses local package tarballs, project planning fails.
Small reproducer:
take any package with tests, e.g.
splitmix(I pick that because it depends only on bundled libraries): https://hackage.haskell.org/package/splitmix-0.1.0.4/splitmix-0.1.0.4.tar.gzcabal.project
foo.cabal
Then if you try to build the project enabling tests, it fails:
The
:test:examplescomponent is a test-suite ofsplitmix. It shouldn't be required or built. This seems to be a regression sincecabal-3.8.1.0.The text was updated successfully, but these errors were encountered: