Apply local configuration to install targets

[alt-romes]

The target of cabal install is not considered to be a local package, which means local configuration (e.g. in cabal.project, or flags like --enable-profiling) does not apply to it.

In 76670eb, we changed the behaviour to applying the local flags to cabal install targets, but it used the literal target string as a package name to which the flags were additionally applied.

However, cabal install targets are NOT necessarily package names, so, e.g., if we did cabal install exe:mycomp, the local flags would not apply since "exe:mycomp" is not a recognized /package/.

The solution is to parse the target selectors first, and apply the local flags to the package of the resolved targets.

Fixes #7297, #8909, the install part of #7236, #8529, #7832

[alt-romes]

Note for tomorrow: I should probably pass --installdir to cabal install in the testsuite in order to clear the warnings out of cabal.out

[gbaz]

Great sleuthing!

[alt-romes]

We should probably backport this

[mpickering]

@alt-romes CI has failed after the rebase, the patch needs to be slightly modified it seems.

[andreabedini]

@alt-romes It looks like this is causing issues. From the matrix channel (I can reproduce).

$ ./cabal install hindent
No cabal.project file or cabal file matching the default glob './*.cabal' was found.

I think the issue is that you cannot call establishProjectBaseContext outside of a project. Indeed it is only called by the withProject subfunction and not by withProject. The function withProjectOrGlobalConfig just tries withProject and if it fails it tries withoutProject. Not the best design.

[alt-romes]

@alt-romes CI has failed after the rebase, the patch needs to be slightly modified it seems.

I noticed now that my rebase force-with-lease deleted the test for remote repository package you added @mpickering.
I've readded it in #9707

[gbaz]

I'm confused/concerned about this despite reviewing it and missing this element (for which, apologies)

My understanding was this was intended to do the following "The solution is to parse the target selectors first, and apply the local flags to the package of the resolved targets."

However it also appears to switch cliConfig to baseConfig which means we take not only command line flags but also project file flags into account (I think?). I don't believe this was discussed in the ticket, and I don't think it is correct behavior?

Why was this change made and was it necessary to resolve the linked issue, or is it just an additional change that was considered an improvement and sort of "slid in"?

[alt-romes]

@gbaz

However it also appears to switch cliConfig to baseConfig which means we take not only command line flags but also project file flags into account (I think?). I don't believe this was discussed in the ticket, and I don't think it is correct behavior?

That's not quite right, before my patch:

    baseCliConfig =
      commandLineFlagsToProjectConfig
        globalFlags
        flags{configFlags = configFlags'}
        clientInstallFlags'
    cliConfig = addLocalConfigToTargets baseCliConfig targetStrings

The change slid in because the addLocalConfigToTargets part only needs to get added by the time that I do add them back, and until then we can work with the base configuration as we don't look at the configuration for any package.

Let me clarify what local options are (excerpt from a commit message that didn't make it):

Currently, there are three kinds of cabal configurations considered when
determining an option of an `ElaboratedConfiguredPackage`:

* Global configuration, in `.cabal/config`

* Local configuration, in
    - Options passed in the cabal invocation, e.g. `cabal build --enable-executable-dynamic`
    - Fields in the top level `cabal.project`, or `cabal.project.local`, e.g. `extra-include-dirs: /opt/homebrew/include`

    Note thus that top-level cabal.project flags and cli flags are
    treated all together at the same level (`local`).

* Per package configuration, as in

    package HsOpenSSL
      extra-include-dirs: /opt/homebrew/Cellar/openssl@3/3.2.0_1/include
      extra-lib-dirs: /opt/homebrew/Cellar/openssl@3/3.2.0_1/lib

Then, we have a definition for whether a package is local to the
project. The local packages are the packages listed in the project which
have a specific source package, rather than just being listed by name in
a `source-repository-stanza`, or in a `package <package-name>` stanza
that configures installed packages.

So, local options are both top-level cabal.project fields and cli arguments.

Now, in 76670eb, the behaviour started to be: apply local options to the targets of cabal install <target>. However, it used whatever the was as a literal package name, and applied the local options to that package. This is wrong e.g. exe:myexe is definitely not a package name, or all:exes...
It wasn't particularly my decision to apply local configuration to cabal install targets. I just fixed behaviour which was inconsistent (e.g. cabal install mypkg --enable-profiling would already install a profiled executable, where as cabal install --enable-profiling, or cabal install myexe --enable-profiling, or cabal install all:exes --enable-profiling would not), and cabal.project top-level fields would already be considered if you did cabal install pkg...

What did this patch actually break, and that @andreabedini fixed subsequently?
Basically, when we specify cabal install all:exes, or all or all:tests, we need to determine all of the packages local to the project in order to install those. I did this by calling establishProjectBaseContext -- which is wrong if done outside of a project. The right thing would be to, if outside of the project, pass an empty list of local packages instead (outside of project, cabal install all would install nothing...). Andrea's patch simply couples a refactor with that.

[alt-romes]

I don't think it would be easy nor necessary to separate the actual cli flags from the top-level cabal.project options to apply to the cabal install target. Honestly, it even feels to me the right behaviour is as is -- to apply both.

If I have a cabal.project

enable-profiling: True

and do cabal install within it, I am expecting a profiled executable.

If I do cabal install <something else> within this project, it is not only that the local options will be applied to that something else, but other things will also be different (withProject vs withoutProject in CmdInstall). I think it is just the nature of the thing.

[tomsmeding]

I don't know the fine details of what kinds of configuration there are, etc., but just reporting as a user here:

• If I'm in a project (i.e. in a directory with a cabal.project file), and I cabal install without further arguments, surely that configuration must be fully taken into account.
• If I'm in a project (idem) and I e.g. cabal install shellcheck, where shellcheck doesn't have anything at all to do with the current project, I do not expect the cabal.project configuration to be taken into account. Global installation shouldn't take local configuration into account. This would also hold if I had added --lib, even with --package-env, because that would be local installation with respect to that particular env file, not with respect to the "current" project.

If you're intending for this to work differently, that's your purview, but I would then at least like a fat warning when using a global installation command in a context where local configuration is used. :)

[alt-romes]

@tomsmeding that sounds about right.

If there is a clear way to distinguish between cli arguments and cabal.project options we could probably look at whether the target refers to a package which is local to the project in order to determine if the cabal.project options should apply. But I'm not sure there is.

If you're intending for this to work differently, that's your purview, but I would then at least like a fat warning when using a global installation command in a context where local configuration is used. :)

FWIW this decision was made in #8779, I have only patched it to work for the remaining cabal install <target> forms.

[gbaz]

Alright, thanks for the clarification @alt-romes and sorry for the confusion on my part. I had wrongly thought #8779 only applied cli flags and not the project file, but some testing with cabal-3.10.1.0 has convinced me I was wrong. I think this is not ideal, but we can live with it, even long term, and there's no reason to attach figuring out if we want to change this in the future to this current patch. (so i did miss it in a review -- but not of this pr but that pr)

Thanks again for walking though it so clearly!

[andreabedini]

If there is a clear way to distinguish between cli arguments and cabal.project options we could probably look at whether the target refers to a package which is local to the project in order to determine if the cabal.project options should apply.

I believe we have something like that: partitionToKnownTargetsAndHackagePackages in CmdInstall.hs