Refactor CmdInstall / fix bug in #9697 #9706

andreabedini · 2024-02-12T08:38:11Z

CmdInstall.installAction is ~300 lines long and full of nested scopes and ad-hoc logic. This change hopes to make it more readable and understandable.

Lift withProject and withoutProject out of installAction and split their relative concerns. E.g. not parsing URIs is installAction's concern not withProject's (which would just return a constant []).
Split an intermediate step into a separate function, resolveTargetSelectorsInProjectBaseContext.
Reuse withGlobalConfig and fromPkgId (renamed)
Fix a bug introduced in Apply local configuration to install targets #9697 802a326 where establishProjectBaseContext is called in a non-project setting. Also simplify its original implementation by moving the change into withProject rather than calling establishProjectBaseContext a second time.

Please read Github PR Conventions and then fill in one of these two templates.

Template Α: This PR modifies cabal behaviour

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.
The documentation has been updated, if necessary.
Manual QA notes have been included.
Tests have been added. (Ask for help if you don’t know how to write them! Ask for an exemption if tests are too complex for too little coverage!)

Template Β: This PR does not modify cabal behaviour (documentation, tests, refactoring, 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).

alt-romes · 2024-02-12T08:41:45Z

Nice! I'll look through this in a moment.

However, I'm concerned about moving the #9697 fix to withProject... this means e.g. cabal install pkg:execomp --enable-profiling of some non-local package will install a not profiled executable.

I'm very fairly certain both cabal install situations need the fix

andreabedini · 2024-02-12T08:44:22Z

However, I'm concerned about moving the #9697 fix to withProject... this means e.g. cabal install pkg:execomp --enable-profiling of some non-local package will install a not profiled executable.

I'm very fairly certain both cabal install situations need the fix

Yes, I am happy to do that (and was indeed wondering ...) I would need to unpack the logic a bit more but if you know what to do please shoot!

alt-romes

The PR in general looks good to me, but the local configuration must be applied to the install target regardless of whether there is a project or not.

I think the right thing to do here is pass an empty list of package names to targetPkgNames when there is no project (it is not a hack, there simply won't be any packages to install if a target such as "all" is used), and when there is a project use the list of packages from the base context.

alt-romes · 2024-02-12T09:53:59Z

@andreabedini can I leave fixing this regression up to you via this MR in its finished state, or should I put up a patch that simply fixes the regression faster? FWIW I'm happy to wait for this small refactor instead of fixing it standalone.

cabal-install/src/Distribution/Client/ScriptUtils.hs

andreabedini · 2024-02-12T17:49:09Z

I added both "with project" and "without project" cases. Note that in both cases I am taking the names from the target selectors (along with the project packages specifiers in the "with project" case).

I think this is correct if we want to add local configuration to the packages that the user asks for. Unless we want to apply the local configuration just like cabal build would.
E.g. in the cabal repo, if I do cabal install --enable-profiling cabal I get:

...
 - hackage-security-0.6.2.4 (lib) (requires download & build)
 - cabal-install-solver-3.11.0.0 (lib) (requires build)
 - cabal-install-3.11.0.0 (lib)  --enable-profiling (requires build)
 - cabal-install-3.11.0.0 (exe:cabal)  --enable-profiling (requires build)

Would you expect cabal-install-solver to also be built with profiling?

(It might be a good idea to do what cabal build would do, but then why are we going through sdists in the first place?)

alt-romes · 2024-02-12T17:52:28Z

I added both "with project" and "without project" cases. Note that in both cases I am taking the names from the target selectors (along with the project packages specifiers in the "with project" case).

Thanks!

...
 - hackage-security-0.6.2.4 (lib) (requires download & build)
 - cabal-install-solver-3.11.0.0 (lib) (requires build)
 - cabal-install-3.11.0.0 (lib)  --enable-profiling (requires build)
 - cabal-install-3.11.0.0 (exe:cabal)  --enable-profiling (requires build)

I think this is exactly the right behaviour, we apply the local configuration to the targets (not the local packages).
The finest level of configuration is per-package, so both the cabal-install lib and exe have to be built with the local configuration. So that happens as expected.

Would you expect cabal-install-solver to also be built with profiling?

So, no.

Thanks again!

andreabedini · 2024-02-13T09:01:09Z

I think this is exactly the right behaviour,

Ok, in this case I think this is ready 👍

alt-romes

Good, thanks for fixing this!

alt-romes

Yes, this is much better in general. Left a few comments.

alt-romes · 2024-02-13T09:15:33Z

cabal-install/src/Distribution/Client/CmdInstall.hs

+  (specs, selectors) <-
+    -- if every selector is already resolved as a packageid, return without further parsing.
+    if null unresolvedTargetStrings
+      then return (parsedSpecifiers, parsedTargets)
+      else do
+        (resolvedSpecifiers, resolvedTargets) <-
+          resolveTargetSelectorsInProjectBaseContext verbosity baseCtx targetStrings targetFilter
+        return (resolvedSpecifiers ++ parsedSpecifiers, resolvedTargets ++ parsedTargets)
+
+  -- Treat all direct targets of install command as local packages, see note in 'installAction'
+  let config =
+        addLocalConfigToPkgs (projectConfig baseCtx) $
+          concatMap (targetPkgNames $ localPackages baseCtx) selectors


We no longer pass specs to addLocalConfigToPkgs, why so? Could you remind me what the specs here are?

If a target string had to be resolved inside the project conterxt, then pkgSpecs will include the project packages turned into source distributions. We want to apply the local configuration only to the actual targets. This was basically what prompted me to clarify the behaviour we wanted. I'll add this as a note.

alt-romes · 2024-02-13T09:19:43Z

cabal-install/src/Distribution/Client/CmdInstall.hs

+    (uris, packageSpecifiers) = partitionEithers $ map woPackageSpecifiers tss
+    packageTargets = map woPackageTargets tss
+
+  -- Treat all direct targets of install command as local packages, see note in 'installAction'


Suggested change

-- Treat all direct targets of install command as local packages, see note in 'installAction'

-- Apply the local configuration (e.g. cli flags) to all direct targets of install command, see note in 'installAction'

andreabedini · 2024-02-13T10:35:57Z

Has the documentation been already updated?

alt-romes · 2024-02-13T10:45:48Z

Has the documentation been already updated?

Which documentation do you mean?

ulysses4ever · 2024-02-15T19:36:52Z

We should make sure this lands asap. The bug it fixes starts hitting people who try to test drive cabal-head (which more and more people are doing, it looks like).

andreasabel · 2024-02-15T22:55:18Z

Can we have a test that something elementary like cabal install some-hackage-package actually starts installing?

master: cabal install from hackage broken? #9715

geekosaur · 2024-02-15T22:58:14Z

#9707

andreabedini · 2024-02-16T05:49:10Z

Adding some comments and fixing a typo. It will be ready soon. w.r.t to tests, there is the one in #9707 but it would be better if we had one in a project context too.

alt-romes · 2024-02-16T06:34:14Z

Adding some comments and fixing a typo. It will be ready soon. w.r.t to tests, there is the one in #9707 but it would be better if we had one in a project context too.

Thanks Andrea, that’s perfect.

@mpickering is working on a test for this. I think we go ahead and merge it in the meanwhile. IIRC there are some difficulties in the test suite to set up a test like this, so Matthew is also fixing that first.

CmdInstall.installAction is ~300 lines long and full of nested scopes and ad-hoc logic. This change hopes to make it more readable and understandable. - Lift withProject and withoutProject out of installAction and split their relative concerns. E.g. not parsing URIs is installAction's concern not withProject's (which would just return a constant []). - Split an intermediate step into a separate function, resolveTargetSelectorsInProjectBaseContext. - Reuse withGlobalConfig and specFromPkgId (renamed from pidPackageSpecifiers). - Avoid trying withProject a second time in case no target is specified. - Fix a bug introduced in 802a326 where establishProjectBaseContext is called in a non-project setting. Also simplify its original implementation by moving the change into withProject rather than calling establishProjectBaseContext a second time. - Document the interaction between cabal v2-install and local configuration and add few comments.

tomsmeding · 2024-02-18T10:27:52Z

Before this PR, when running cabal install parsec (in a directory with a .cabal file because of the bug reported earlier), I would get the following error:

Error: [Cabal-7127]
Cannot build the executables in the package parsec because it does not contain any executables. Check the .cabal file for the package and make sure that it properly declares the components that you expect.

and that was it. Adding --lib would make it silently succeed and create an environment file.

After this PR, the same cabal install parsec (now also outside of a project!) produces the same error in a project context, but outside of a project I get this:

Resolving dependencies...
Warning:
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
@ WARNING: Installation might not be completed as desired! @
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
The command "cabal install [TARGETS]" doesn't expose libraries.
* You might have wanted to add them as dependencies to your package. In this
case add "parsec" to the build-depends field(s) of your package's .cabal file.
* You might have wanted to add them to a GHC environment. In this case use
"cabal install --lib parsec". The "--lib" flag is provisional: see
https://github.com/haskell/cabal/issues/6481 for more information.
Warning:
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
@ WARNING: Installation might not be completed as desired! @
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
The command "cabal install [TARGETS]" doesn't expose libraries.
* You might have wanted to add them as dependencies to your package. In this
case add "parsec" to the build-depends field(s) of your package's .cabal file.
* You might have wanted to add them to a GHC environment. In this case use
"cabal install --lib parsec". The "--lib" flag is provisional: see
https://github.com/haskell/cabal/issues/6481 for more information.

Why do I get a different message inside and outside a project context? I'm doing the same thing.
Why do I get the message twice?

andreabedini · 2024-02-18T13:49:13Z

I am going to look at this tomorrow (and thank you for testing!)

andreabedini · 2024-02-19T01:00:33Z

@tomsmeding I am afraid I cannot reproduce.

I am in a directory with a cabal file.

~/Scratchpad/cabal-9706
❯ ls -l
total 8
drwxr-xr-x 1 andrea andrea  14 Feb 19 07:39 app
-rw-r--r-- 1 andrea andrea 421 Feb 19 07:39 cabal9706.cabal
-rw-r--r-- 1 andrea andrea 112 Feb 19 07:39 CHANGELOG.md

and I get (with and without --lib):

~/Scratchpad/cabal-9706
❯ just $HOME/code/cabal/master/run install parsec --dry-run
Up to date
Running: /home/andrea/code/cabal/master/dist-newstyle/build/x86_64-linux/ghc-9.6.4/cabal-install-3.11.0.0/x/cabal/build/cabal/cabal install parsec --dry-run
Warning: this is a debug build of cabal-install with assertions enabled.
Resolving dependencies...
Error: [Cabal-7127]
Cannot build the executables in the package parsec because it does not contain any executables. Check the .cabal file for the package and make sure that it properly declares the components that you expect.

error: Recipe `run` failed on line 113 with exit code 1

~/Scratchpad/cabal-9706
❯ just $HOME/code/cabal/master/run install parsec --dry-run --lib
Up to date
Running: /home/andrea/code/cabal/master/dist-newstyle/build/x86_64-linux/ghc-9.6.4/cabal-install-3.11.0.0/x/cabal/build/cabal/cabal install parsec --dry-run --lib
Warning: this is a debug build of cabal-install with assertions enabled.
Resolving dependencies...
Build profile: -w ghc-9.6.4 -O1
In order, the following would be built (use -v for more details):
 - parsec-3.1.17.0 (lib) (requires download & build)

This is the same beaviour as the current release:

~/Scratchpad/cabal-9706
❯ cabal install parsec --dry-run
Resolving dependencies...
Error: cabal: Cannot build the executables in the package parsec because it
does not contain any executables. Check the .cabal file for the package and
make sure that it properly declares the components that you expect.

~/Scratchpad/cabal-9706
❯ cabal install parsec --dry-run --lib
Resolving dependencies...
Build profile: -w ghc-9.6.4 -O1
In order, the following would be built (use -v for more details):
 - parsec-3.1.17.0 (lib) (requires build)

~/Scratchpad/cabal-9706
❯ cabal --version
cabal-install version 3.10.2.1
compiled using version 3.10.2.1 of the Cabal library

andreabedini · 2024-02-19T01:04:34Z

Why do I get a different message inside and outside a project context? I'm doing the same thing.

Well, there is a reason. As we decided to honour project configuration in cabal install (in #9697); the version and configuration of parsec now depend on the project so you can aspect different results. To ignore the project configuration you can use --ignore-project.

Why do I get the message twice?

That would be a bug!

Do you have a minimal reproducible example I can try?

gbaz · 2024-02-19T01:19:17Z

eep, i just realized that #9697 means we apply cabal.project configuration to cabal install commands. I don't think that's correct! I believe cabal install should work the same no matter if there's a project file around or not -- however, it should take command line flags.

This is to say that cabal install (especially of a package from a remote repo, but really from anywhere) should work the same no matter what directory it is in, as it isn't installing into a project-local context but either an env file (for a lib) or the globally available /usr/bin or the like.

But if course we want to allow flags to affect the result of install -- which was the initial thing we set out to fix, no?

alt-romes · 2024-02-19T07:45:15Z

@gbaz I've clarified and replied to that comment on #9697 (comment)

tomsmeding · 2024-02-19T08:19:12Z

@andreabedini Here is cabal install parsec and cabal install parsec --lib in a directory with a *.cabal file and in a directory without, with cabal 3.10.2.1, cabal at fe82d9bce (before this PR; call this "cabal-fe") and cabal at ccab6d15f (the merge commit of this PR; call this "cabal-cc").

https://paste.tomsmeding.com/VQiFSDpi/raw/1

Notes:

For cabal 3.10.2.1 and cabal-cc: cabal install parsec gives a different error message inside and outside a project. This is my (1.) above.
cabal-fe works the same as cabal 3.10.2.1 inside a project, but refuses to do anything outside. This is my earlier bug report from matrix.
cabal-cc has the same behaviour as 3.10.2.1, except that the "outside" message is printed twice. (This is my (2.) above.)

It looks like my (1.) (different error message for cabal install parsec inside and outside a project) is something that is old and already present in the released 3.10.2.1. However, (2.) is at least newer than 3.10.2.1.

I have not git bisected (2.). I guess you can do this as well as I can, and I'll be busy today, sorry.

andreabedini · 2024-03-19T02:50:05Z

@tomsmeding Could you please open a separate issue if this is still a problem? (and if you haven't already)

andreabedini requested a review from alt-romes February 12, 2024 08:38

andreabedini added the attention: needs-review label Feb 12, 2024

andreabedini force-pushed the refactor/cmd-install branch from edf950b to ffe8d2c Compare February 12, 2024 08:40

alt-romes reviewed Feb 12, 2024

View reviewed changes

cabal-install/src/Distribution/Client/ScriptUtils.hs Show resolved Hide resolved

andreabedini force-pushed the refactor/cmd-install branch from ffe8d2c to 4de55c1 Compare February 12, 2024 17:35

andreabedini marked this pull request as ready for review February 12, 2024 17:49

andreabedini force-pushed the refactor/cmd-install branch from 4de55c1 to e0ee667 Compare February 13, 2024 09:01

alt-romes approved these changes Feb 13, 2024

View reviewed changes

alt-romes reviewed Feb 13, 2024

View reviewed changes

geekosaur mentioned this pull request Feb 15, 2024

master: cabal install from hackage broken? #9715

Closed

geekosaur linked an issue Feb 15, 2024 that may be closed by this pull request

master: cabal install from hackage broken? #9715

Closed

geekosaur approved these changes Feb 15, 2024

View reviewed changes

andreabedini force-pushed the refactor/cmd-install branch 2 times, most recently from 4d55140 to 656d06b Compare February 16, 2024 06:16

andreabedini added merge me Tell Mergify Bot to merge and removed attention: needs-review labels Feb 16, 2024

alt-romes mentioned this pull request Feb 16, 2024

Fix cabal install in the presence of extra-packages #9719

Merged

mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Feb 18, 2024

Mikolaj force-pushed the refactor/cmd-install branch from 656d06b to 7b1746f Compare February 18, 2024 06:49

mergify bot merged commit ccab6d1 into haskell:master Feb 18, 2024
50 checks passed

tomsmeding mentioned this pull request Feb 18, 2024

Add a warning when an env file is created #9705

Merged

2 tasks

tomsmeding mentioned this pull request Mar 25, 2024

cabal install parsec error inconsistent inside and outside project #9839

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor CmdInstall / fix bug in #9697 #9706

Refactor CmdInstall / fix bug in #9697 #9706

andreabedini commented Feb 12, 2024

alt-romes commented Feb 12, 2024

andreabedini commented Feb 12, 2024

alt-romes left a comment •

edited

Loading

alt-romes commented Feb 12, 2024

andreabedini commented Feb 12, 2024

alt-romes commented Feb 12, 2024 •

edited

Loading

andreabedini commented Feb 13, 2024

alt-romes left a comment

alt-romes left a comment

alt-romes Feb 13, 2024

andreabedini Feb 16, 2024

alt-romes Feb 13, 2024

andreabedini commented Feb 13, 2024

alt-romes commented Feb 13, 2024

ulysses4ever commented Feb 15, 2024 •

edited

Loading

andreasabel commented Feb 15, 2024

geekosaur commented Feb 15, 2024

andreabedini commented Feb 16, 2024

alt-romes commented Feb 16, 2024

tomsmeding commented Feb 18, 2024

andreabedini commented Feb 18, 2024 •

edited

Loading

andreabedini commented Feb 19, 2024

andreabedini commented Feb 19, 2024

gbaz commented Feb 19, 2024

alt-romes commented Feb 19, 2024 •

edited

Loading

tomsmeding commented Feb 19, 2024

andreabedini commented Mar 19, 2024

	-- Treat all direct targets of install command as local packages, see note in 'installAction'
	-- Apply the local configuration (e.g. cli flags) to all direct targets of install command, see note in 'installAction'

Refactor CmdInstall / fix bug in #9697 #9706

Refactor CmdInstall / fix bug in #9697 #9706

Conversation

andreabedini commented Feb 12, 2024

alt-romes commented Feb 12, 2024

andreabedini commented Feb 12, 2024

alt-romes left a comment • edited Loading

Choose a reason for hiding this comment

alt-romes commented Feb 12, 2024

andreabedini commented Feb 12, 2024

alt-romes commented Feb 12, 2024 • edited Loading

andreabedini commented Feb 13, 2024

alt-romes left a comment

Choose a reason for hiding this comment

alt-romes left a comment

Choose a reason for hiding this comment

alt-romes Feb 13, 2024

Choose a reason for hiding this comment

andreabedini Feb 16, 2024

Choose a reason for hiding this comment

alt-romes Feb 13, 2024

Choose a reason for hiding this comment

andreabedini commented Feb 13, 2024

alt-romes commented Feb 13, 2024

ulysses4ever commented Feb 15, 2024 • edited Loading

andreasabel commented Feb 15, 2024

geekosaur commented Feb 15, 2024

andreabedini commented Feb 16, 2024

alt-romes commented Feb 16, 2024

tomsmeding commented Feb 18, 2024

andreabedini commented Feb 18, 2024 • edited Loading

andreabedini commented Feb 19, 2024

andreabedini commented Feb 19, 2024

gbaz commented Feb 19, 2024

alt-romes commented Feb 19, 2024 • edited Loading

tomsmeding commented Feb 19, 2024

andreabedini commented Mar 19, 2024

alt-romes left a comment •

edited

Loading

alt-romes commented Feb 12, 2024 •

edited

Loading

ulysses4ever commented Feb 15, 2024 •

edited

Loading

andreabedini commented Feb 18, 2024 •

edited

Loading

alt-romes commented Feb 19, 2024 •

edited

Loading