Consider Installed packages when pruning the install plan #6972

alexbiehl · 2020-07-19T15:55:19Z

The discussion in #6952 indicated that extra-packages stanzas wouldn't quite work yet. It turns out in order for cabal to find exes for already installed extra-packages we need to also consider installed packages when pruning the install plan.

Here is what you can do with it:

diff --git a/cabal.project b/cabal.project
index af1d8a383..b1d724259 100644
--- a/cabal.project
+++ b/cabal.project
@@ -7,6 +7,8 @@ packages: Cabal/Cabal-tree-diff/
 packages: Cabal/Cabal-described
 packages: cabal-benchmarks/
 
+extra-packages: stylish-haskell
+
 -- Uncomment to allow picking up extra local unpacked deps:
 --optional-packages: */

Using HEAD

$ cabal run cabal-install -- run -- stylish-haskell --version
Up to date
cabal: Unknown executable stylish-haskell in package
stylsh-hskll-0.9.3.0-cdf9f8e3

With this patch applied

$ cabal run cabal-install -- run -- stylish-haskell --version
Up to date
stylish-haskell 0.9.3.0

Come up with a test case that doesn't need to depend on an external package :S

Please include the following checklist in your PR:

Patches conform to the coding conventions.
Any changes that could be relevant to users have been recorded in the changelog (add file to changelog.d directory).
The documentation has been updated, if necessary.

Please also shortly describe how you tested your change. Bonus points for added tests!

phadej · 2020-07-19T21:13:31Z

For tests, run --dry-run should already do something. There are tools (e.g. alex, happy) which are buildable with ~any GHC version we ever test with.

phadej · 2020-07-19T21:14:40Z

I also don't see changelog file. Did you forget to git add it?

alexbiehl · 2020-07-20T11:37:49Z

I am having troubles with the patch. The integration test wants me to exactly specify the unit id of the happy package. But that's pretty much environment dependent. Need to investigate more. And maybe I am blind but is there maybe simpler test suite already that let's me run cabal and have a fixture for the expected output?

phadej · 2020-07-20T11:55:52Z

Look at cabal-testsuite tests. There are plenty. Remember to add cabal.project file (otherwise this repositories top-level is picked).

alexbiehl · 2020-07-27T16:07:18Z

I am seeing

cabal: Could not resolve dependencies:
[__0] unknown package: alex (user goal)
[__0] fail (backjumping, conflict set: alex)
After searching the rest of the dependency tree exhaustively, these were the goals I've had most trouble fulfilling: alex (1)

with my test case. Are we executing tests with an index?

phadej · 2020-07-27T17:13:37Z

@alexbiehl looks like so. Grep for withRepo for examples of tests with package repositories.

phadej · 2020-07-27T22:24:41Z

Hmm, tests pass because

PackageTests/ExtraPackages/cabal.test.hs                                                                          SKIP (0.02s)

you actually need to add alex-... package to repo/ directory.

I have to fix #6820 and install hackage-repo-tool onto CI runners. (EDIT: or start using file+noindex repos for them!!!)

alexbiehl · 2020-09-02T17:44:14Z

@phadej Do you think we can move this patch forward? I don't think we should wait for the test suite to be refactored to get this change in. I can drop the failed attempt at tests for now. It would be beneficial to have this patch in the upcoming cabal-install release.

phadej · 2020-09-02T17:48:31Z

Sorry. I have experienced too many times that tests are not written retroactively.

alexbiehl · 2020-09-02T17:50:56Z

Alright, do we have a clear path for a test? It seems that my attempts resulted in the tests being skipped as withRepo doesn't seem to work properly on the test machines?

The discussion in haskell#6952 indicated that extra-packages stanzas wouldn't quite work yet. It turns out in order for cabal to find exes for already installed extra-packages we need to also consider `installed` packages when pruning the install plan.

cabal-testsuite/PackageTests/ExtraPackages/cabal.out

alexbiehl · 2020-09-14T15:30:07Z

Alright! I think tests are working properly now. @phadej mind giving this another look?

phadej · 2020-09-14T16:31:35Z

cabal-testsuite/src/Test/Cabal/OutputNormalizer.hs

+  -- Install path frequently has architecture specific elements, so
+  -- nub it out
+  . resub "^Warning: The directory (.+) is not in the system search path."
+          "Warning: The directory <PATH> is not in the system search path."


I think this hides a bug which you don't address.

Why extra-packages is build as local package, and run from somewhere inside dist-newstyle and not installed to the store and run from there.

Or so it looked like in previous CI errors.

I'm 👎 on this change. It shouldn't be required.

Why extra-packages is build as local package, and run from somewhere inside dist-newstyle and not installed to the store and run from there.

Hm, that is not what's happening. extra-packages are not treated as local. extra-package are installed globally into the store which results in displaying the store-path in above message. Unfortunately store-paths are platform dependent, hence the above change.

I think we should rather teach the store-dir location and replace that with <STOREDIR> (as we already do for other platform dependent things), rather then hiding big parts of the output

So if path is

.../store/ghc-8.6.5/Agda-2.6.2-18fb8b49b1f47de3065c0ae3a191d1060c3959e9293aeb9e2aa88c3705c7a92d/bin/agda

we know how to replace ghc-8.6.5, matching against /package-id-hash/ and replacing just hash part with <HASH> should work. An approximation of package-id is not tricky. See unqual-name and version in https://cabal.readthedocs.io/en/3.4/buildinfo-fields-reference.html#non-terminals

Right, for other store path occurrences we are using the same patterns:

-- Install path frequently has architecture specific elements, so -- nub it out . resub "Installing (.+) in .+" "Installing \\1 in <PATH>"

phadej · 2020-09-14T17:18:54Z

I tried this patch locally, and it works. don't like what we print

Installing executable cabal-fmt in /cabal/store/ghc-8.6.5/incoming/new-52431/cabal/store/ghc-8.6.5/cabal-fmt-0.1.2-e-cabal-fmt-c0266ba350e909c32b666048e3fc6b2c0db976621f3da582284e8b390d97c14b/bin
Warning: The directory
/cabal/store/ghc-8.6.5/incoming/new-52431/cabal/store/ghc-8.6.5/cabal-fmt-0.1.2-e-cabal-fmt-c0266ba350e909c32b666048e3fc6b2c0db976621f3da582284e8b390d97c14b/bin
is not in the system search path.

as that incoming path shouldn't be printed. The command is run from /cabal/store/ghc-8.6.5/cabal-fmt-0.1.2-e-cabal-fmt-c0266ba350e909c32b666048e3fc6b2c0db976621f3da582284e8b390d97c14b/bin/cabal-fmt

which is more sensible path (btw it might make sense to recognise just unqual-name-hash, as looks like we make fancy unit-ids, sorry I forgot about that :) )

That "not in the system path" comes from Distribution.Simple.Install and is irrelevant for the users. Luckily it's printed only on higher verbosity levels, but it's still confusing.

phadej · 2020-09-14T18:17:38Z

diff --git a/cabal-testsuite/PackageTests/ExtraPackages/cabal.out b/cabal-testsuite/PackageTests/ExtraPackages/cabal.out
index 7820c030c..b82be3336 100644
--- a/cabal-testsuite/PackageTests/ExtraPackages/cabal.out
+++ b/cabal-testsuite/PackageTests/ExtraPackages/cabal.out
@@ -9,4 +9,4 @@ Configuring some-exe-0.0.1.0...
 Preprocessing executable 'some-exe' for some-exe-0.0.1.0..
 Building executable 'some-exe' for some-exe-0.0.1.0..
 Installing executable some-exe in <PATH>
-Warning: The directory <PATH> is not in the system search path.
+Warning: The directory <ROOT>/cabal.dist/home/.cabal/store/ghc-<GHCVER>/incoming/new-<HASH><ROOT>/cabal.dist/home/.cabal/store/ghc-<GHCVER>/some-exe-0.0.1.0-<HASH>/bin is not in the system search path.
diff --git a/cabal-testsuite/src/Test/Cabal/OutputNormalizer.hs b/cabal-testsuite/src/Test/Cabal/OutputNormalizer.hs
index e4bb63f61..e48776ea7 100644
--- a/cabal-testsuite/src/Test/Cabal/OutputNormalizer.hs
+++ b/cabal-testsuite/src/Test/Cabal/OutputNormalizer.hs
@@ -30,7 +30,7 @@ normalizeOutput nenv =
     -- This is dumb but I don't feel like pulling in another dep for
     -- string search-replace.  Make sure we do this before backslash
     -- normalization!
-  . resub (posixRegexEscape (normalizerGblTmpDir nenv) ++ "[a-z0-9.-]+") "<GBLTMPDIR>" -- note, after TMPDIR
+  . resub (posixRegexEscape (normalizerGblTmpDir nenv) ++ "[a-z0-9\\.-]+") "<GBLTMPDIR>" -- note, after TMPDIR
   . resub (posixRegexEscape (normalizerRoot nenv)) "<ROOT>/"
   . resub (posixRegexEscape (normalizerTmpDir nenv)) "<TMPDIR>/"
   . appEndo (F.fold (map (Endo . packageIdRegex) (normalizerKnownPackages nenv)))
@@ -39,6 +39,12 @@ normalizeOutput nenv =
     -- Apply this before packageIdRegex, otherwise this regex doesn't match.
   . resub "[0-9]+(\\.[0-9]+)*/installed-[A-Za-z0-9.+]+"
           "<VERSION>/installed-<HASH>"
+    -- incoming directories in the store
+  . resub "/incoming/new-[0-9]+"
+          "/incoming/new-<RAND>"
+    -- look for PackageHash directories
+  . resub "/(([A-Za-z0-9]+)(-[A-Za-z0-9\\.]+)*)-[0-9a-f]{4,64}/"
+          "/\\1-<HASH>/"
     -- Normalize architecture
   . resub (posixRegexEscape (display (normalizerPlatform nenv))) "<ARCH>"
     -- Some GHC versions are chattier than others
@@ -54,10 +60,6 @@ normalizeOutput nenv =
         else id)
   -- hackage-security locks occur non-deterministically
   . resub "(Released|Acquired|Waiting) .*hackage-security-lock\n" ""
-  -- Install path frequently has architecture specific elements, so
-  -- nub it out
-  . resub "^Warning: The directory (.+) is not in the system search path."
-          "Warning: The directory <PATH> is not in the system search path."
   where
     packageIdRegex pid =
         resub (posixRegexEscape (display pid) ++ "(-[A-Za-z0-9.-]+)?")

phadej · 2020-09-14T18:20:24Z

I'm not sure whether new-[0-9]+ is good enough for all architectures, but I hope it is.

alexbiehl · 2020-09-15T08:35:31Z

Cool! Unfortunately we have to escape the package name as well as it differs on Linux and OSX:

On my mac machine it is

sm-x-0.0.1.0-<HASH>/bin

On CI it is

some-exe-0.0.1.0-HASH/bin

alexbiehl · 2020-09-15T08:53:26Z

cabal-testsuite/PackageTests/ExtraPackages/cabal.out

@@ -9,4 +9,4 @@ Configuring some-exe-0.0.1.0...
 Preprocessing executable 'some-exe' for some-exe-0.0.1.0..
 Building executable 'some-exe' for some-exe-0.0.1.0..
 Installing executable some-exe in <PATH>
-Warning: The directory <ROOT>/cabal.dist/home/.cabal/store/ghc-<GHCVER>/incoming/new-<HASH><ROOT>/cabal.dist/home/.cabal/store/ghc-<GHCVER>/sm-x-0.0.1.0-<HASH>/bin is not in the system search path.
+Warning: The directory <ROOT>/cabal.dist/home/.cabal/store/ghc-<GHCVER>/incoming/<PACKAGE>-<HASH><ROOT>/cabal.dist/home/.cabal/store/ghc-<GHCVER>/<PACKAGE>-<HASH>/bin is not in the system search path.


Argl it escapes too mucho :(

phadej · 2020-09-15T08:58:11Z

hmm. that some-exe vs sm-x is tricky. Let me think what can be done there.

alexbiehl · 2020-09-17T07:44:29Z

What about trimming away the whole path in that log message? Its a lot easier than surgically replacing butchered package names..

phadej · 2020-09-17T08:07:48Z

What about trimming away the whole path in that log message? Its a lot easier than surgically replacing butchered package names.

No. I said it already. We want to catch cases where we e.g. reorganise something.

I'll look at this next month, I'm running out of time to work on Cabal.

phadej · 2020-10-03T23:08:31Z

I made more tweaks to OutputNormalizer, and got tests to be green in #7099

alexbiehl mentioned this pull request Jul 19, 2020

Teach cabal.project about "dev dependencies" #6952

Closed

alexbiehl force-pushed the alex/extra-packages-target-selectors branch from f7aeba5 to e83094f Compare July 20, 2020 13:01

Alex Biehl added 2 commits September 14, 2020 10:39

Changelog entry

129d64b

alexbiehl force-pushed the alex/extra-packages-target-selectors branch 5 times, most recently from 2050e8a to ad73e2a Compare September 14, 2020 09:36

alexbiehl commented Sep 14, 2020

View reviewed changes

cabal-testsuite/PackageTests/ExtraPackages/cabal.out Outdated Show resolved Hide resolved

alexbiehl force-pushed the alex/extra-packages-target-selectors branch 3 times, most recently from 89dbc1c to 43bb404 Compare September 14, 2020 15:29

phadej suggested changes Sep 14, 2020

View reviewed changes

Alex Biehl added 2 commits September 15, 2020 10:30

Escape store incoming directory

335a56f

Test extra-packages

4bb674d

alexbiehl force-pushed the alex/extra-packages-target-selectors branch from 43bb404 to 4bb674d Compare September 15, 2020 08:31

Properly escape package in store path

0d094ac

alexbiehl commented Sep 15, 2020

View reviewed changes

This was referenced Oct 2, 2020

Pr 6972 extra packages #7099

Merged

cabal-install >= 3.4 regression: cabal repl -b fails to install packages in combination with --allow-newer #7081

Closed

phadej closed this Oct 3, 2020

alexbiehl deleted the alex/extra-packages-target-selectors branch May 25, 2021 13:32

phadej mentioned this pull request Oct 4, 2021

Fix #7698: cabal sdist works when extra-packages are present #7699

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consider Installed packages when pruning the install plan #6972

Consider Installed packages when pruning the install plan #6972

alexbiehl commented Jul 19, 2020

phadej commented Jul 19, 2020

phadej commented Jul 19, 2020

alexbiehl commented Jul 20, 2020

phadej commented Jul 20, 2020

alexbiehl commented Jul 27, 2020

phadej commented Jul 27, 2020

phadej commented Jul 27, 2020 •

edited

Loading

alexbiehl commented Sep 2, 2020

phadej commented Sep 2, 2020

alexbiehl commented Sep 2, 2020

alexbiehl commented Sep 14, 2020

phadej Sep 14, 2020

alexbiehl Sep 14, 2020 •

edited

Loading

phadej Sep 14, 2020

phadej Sep 14, 2020 •

edited

Loading

alexbiehl Sep 14, 2020

phadej commented Sep 14, 2020

phadej commented Sep 14, 2020 •

edited

Loading

phadej commented Sep 14, 2020

alexbiehl commented Sep 15, 2020

alexbiehl Sep 15, 2020

phadej commented Sep 15, 2020

alexbiehl commented Sep 17, 2020

phadej commented Sep 17, 2020 •

edited

Loading

phadej commented Oct 3, 2020

Consider Installed packages when pruning the install plan #6972

Consider Installed packages when pruning the install plan #6972

Conversation

alexbiehl commented Jul 19, 2020

phadej commented Jul 19, 2020

phadej commented Jul 19, 2020

alexbiehl commented Jul 20, 2020

phadej commented Jul 20, 2020

alexbiehl commented Jul 27, 2020

phadej commented Jul 27, 2020

phadej commented Jul 27, 2020 • edited Loading

alexbiehl commented Sep 2, 2020

phadej commented Sep 2, 2020

alexbiehl commented Sep 2, 2020

alexbiehl commented Sep 14, 2020

phadej Sep 14, 2020

Choose a reason for hiding this comment

alexbiehl Sep 14, 2020 • edited Loading

Choose a reason for hiding this comment

phadej Sep 14, 2020

Choose a reason for hiding this comment

phadej Sep 14, 2020 • edited Loading

Choose a reason for hiding this comment

alexbiehl Sep 14, 2020

Choose a reason for hiding this comment

phadej commented Sep 14, 2020

phadej commented Sep 14, 2020 • edited Loading

phadej commented Sep 14, 2020

alexbiehl commented Sep 15, 2020

alexbiehl Sep 15, 2020

Choose a reason for hiding this comment

phadej commented Sep 15, 2020

alexbiehl commented Sep 17, 2020

phadej commented Sep 17, 2020 • edited Loading

phadej commented Oct 3, 2020

phadej commented Jul 27, 2020 •

edited

Loading

alexbiehl Sep 14, 2020 •

edited

Loading

phadej Sep 14, 2020 •

edited

Loading

phadej commented Sep 14, 2020 •

edited

Loading

phadej commented Sep 17, 2020 •

edited

Loading