Take extra program search paths into account when searching for VCS programs.

[arrowd]

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

This patch fixes a TODO in syncAndReadSourcePackagesRemoteRepos. Although I was expecting that progPathExtra corresponds to --extra-prog-path command line option, it turned out that it is always set to ~/.cabal/bin. Anyways, this makes cabal-install look into this dir first when searching for VCS executables.

To test this create a following cabal.project:

source-repository-package
    type: bazaar
    location: https://github.com/foo/bar
    tag: e70cf0c171c9a586b62b3f75d72f1591e4e6aaa1

Most likely you don't have Bazaar, so cabal build will fail. Now run ln -s /usr/bin/true ~/.cabal/bin/bzr and cabal-install will find it.

[Mikolaj]

LGTM, though it also needs a test (half-dummy is enough; even just that adding extra paths doesn't suddenly crash cabal after this change; or that mentioning the main path as the extra path doesn't cancel it out) and a changelog file.

[arrowd]

How do I write a test for that?

[ulysses4ever]

Although I was expecting that progPathExtra corresponds to --extra-prog-path command line option, it turned out that it is always set to ~/.cabal/bin

This sounds strange. Maybe instead of using progPathExtra (as the Todo suggests) you should figure which field carries the value of --extra-prog-path and use it? It'd make more sense imho and should be easier to test too.

[ulysses4ever]

Maybe not "instead" but "in addition to" rather.

[arrowd]

Ok, this is the ProjectConfig value I dumped from the cabal-install configure --extra-prog-path=/asd:

ProjectConfig
  { projectPackages = [],
    projectPackagesOptional = [],
    projectPackagesRepo =
      [ SourceRepositoryPackage
          { srpType = KnownRepoType Git,
            srpLocation = "https://github.com/denisshevchenko/threepenny-gui",
            srpTag = Just "4ec92ded05ccf59ba4a874be4b404ac1b6d666b6",
            srpBranch = Nothing,
            srpSubdir = [],
            srpCommand = []
          }
      ],
    projectPackagesNamed = [],
    projectConfigBuildOnly =
      ProjectConfigBuildOnly
        { projectConfigVerbosity = Flag (Verbosity {vLevel = Normal, vFlags = fromList [], vQuiet = False}),
          projectConfigDryRun = Flag False,
          projectConfigOnlyDeps = Flag False,
          projectConfigOnlyDownload = Flag False,
          projectConfigSummaryFile = ["/home/arr/.cabal/logs/build.log"],
          projectConfigLogFile = NoFlag,
          projectConfigBuildReports = Flag AnonymousReports,
          projectConfigReportPlanningFailure = Flag False,
          projectConfigSymlinkBinDir = NoFlag,
          projectConfigNumJobs = Flag Nothing,
          projectConfigKeepGoing = Flag False,
          projectConfigOfflineMode = Flag False,
          projectConfigKeepTempFiles =
            Flag False,
          projectConfigHttpTransport = NoFlag,
          projectConfigIgnoreExpiry = NoFlag,
          projectConfigCacheDir = Flag "/home/arr/.cabal/packages",
          projectConfigLogsDir = Flag "/home/arr/.cabal/logs",
          projectConfigClientInstallFlags =
            ClientInstallFlags
              { cinstInstallLibs = Flag False,
                cinstEnvironmentPath = NoFlag,
                cinstOverwritePolicy = NoFlag,
                cinstInstallMethod = NoFlag,
                cinstInstalldir = Flag "/home/arr/.cabal/bin"
              }
        },
    projectConfigShared =
      ProjectConfigShared
        { projectConfigDistDir = NoFlag,
          projectConfigConfigFile = NoFlag,
          projectConfigProjectFile = NoFlag,
          projectConfigIgnoreProject = Flag False,
          projectConfigHcFlavor = Flag GHC,
          projectConfigHcPath = NoFlag,
          projectConfigHcPkg = NoFlag,
          projectConfigHaddockIndex = Flag "$datadir/doc/$arch-$os-$compiler/index.html",
          projectConfigPackageDBs = [],
          projectConfigRemoteRepos = [RemoteRepo {remoteRepoName = RepoName {unRepoName = "hackage.haskell.org"}, remoteRepoURI = http :// hackage . haskell . org /, remoteRepoSecure = Just True, remoteRepoRootKeys = ["fe331502606802feac15e514d9b9ea83fee8b6ffef71335479a2e68d84adc6b0", "1ea9ba32c526d1cc91ab5e5bd364ec5e9e8cb67179a471872f6e26f0ae773d42", "2c6c3627bd6c982990239487f1abd02e08a02e6cf16edb105a8012d444d870c3", "0a5c7ea47cd1b15f01f5f51a33adda7e655bc0f0b0615baa8e271f4c3351e21d", "51f0161b906011b52c6613376b1ae937670da69322113a246a09f807c62f6921"], remoteRepoKeyThreshold = 3, remoteRepoShouldTryHttps = True}],
          projectConfigLocalNoIndexRepos = [],
          projectConfigActiveRepos = NoFlag,
          projectConfigIndexState = NoFlag,
          projectConfigStoreDir = NoFlag,
          projectConfigConstraints = [],
          projectConfigPreferences = [],
          projectConfigCabalVersion = NoFlag,
          projectConfigSolver = Flag AlwaysModular,
          projectConfigAllowOlder = Nothing,
          projectConfigAllowNewer = Nothing,
          projectConfigWriteGhcEnvironmentFilesPolicy = NoFlag,
          projectConfigMaxBackjumps = Flag 4000,
          projectConfigReorderGoals = Flag (ReorderGoals False),
          projectConfigCountConflicts = Flag (CountConflicts True),
          projectConfigFineGrainedConflicts = Flag (FineGrainedConflicts True),
          projectConfigMinimizeConflictSet = Flag (MinimizeConflictSet False),
          projectConfigStrongFlags = Flag (StrongFlags False),
          projectConfigAllowBootLibInstalls = Flag (AllowBootLibInstalls False),
          projectConfigOnlyConstrained = Flag OnlyConstrainedNone,
          projectConfigPerComponent = Flag True,
          projectConfigIndependentGoals = Flag (IndependentGoals False),
          projectConfigPreferOldest = Flag (PreferOldest False),
          projectConfigProgPathExtra = ["/home/arr/.cabal/bin"]
        },
    projectConfigProvenance = fromList [Explicit "/usr/home/arr/aaa/cabal.project", Explicit "/usr/home/arr/aaa/cabal.project.local"],
    projectConfigAllPackages =
      PackageConfig
        { packageConfigProgramPaths = MapLast {getMapLast = fromList []},
          packageConfigProgramArgs = MapMappend {getMapMappend = fromList []},
          packageConfigProgramPathExtra =
            [ "/home/arr/.cabal/bin",
              "/asd"
            ],
          packageConfigFlagAssignment = fromList [],
          packageConfigVanillaLib = NoFlag,
          packageConfigSharedLib = NoFlag,
          packageConfigStaticLib = NoFlag,
          packageConfigDynExe = NoFlag,
          packageConfigFullyStaticExe = NoFlag,
          packageConfigProf = NoFlag,
          packageConfigProfLib = NoFlag,
          packageConfigProfExe = NoFlag,
          packageConfigProfDetail = NoFlag,
          packageConfigProfLibDetail = NoFlag,
          packageConfigConfigureArgs = [],
          packageConfigOptimization = NoFlag,
          packageConfigProgPrefix = NoFlag,
          packageConfigProgSuffix = NoFlag,
          packageConfigExtraLibDirs = [],
          packageConfigExtraLibDirsStatic = [],
          packageConfigExtraFrameworkDirs = [],
          packageConfigExtraIncludeDirs = [],
          packageConfigGHCiLib = NoFlag,
          packageConfigSplitSections = NoFlag,
          packageConfigSplitObjs = NoFlag,
          packageConfigStripExes = NoFlag,
          packageConfigStripLibs = NoFlag,
          packageConfigTests = NoFlag,
          packageConfigBenchmarks = NoFlag,
          packageConfigCoverage = NoFlag,
          packageConfigRelocatable = NoFlag,
          packageConfigDebugInfo = NoFlag,
          packageConfigDumpBuildInfo = NoFlag,
          packageConfigRunTests = NoFlag,
          packageConfigDocumentation = Flag False,
          packageConfigHaddockHoogle = Flag False,
          packageConfigHaddockHtml = Flag False,
          packageConfigHaddockHtmlLocation = NoFlag,
          packageConfigHaddockForeignLibs = Flag False,
          packageConfigHaddockExecutables = Flag False,
          packageConfigHaddockTestSuites = Flag False,
          packageConfigHaddockBenchmarks = Flag False,
          packageConfigHaddockInternal = Flag False,
          packageConfigHaddockCss = NoFlag,
          packageConfigHaddockLinkedSource = Flag False,
          packageConfigHaddockQuickJump = Flag False,
          packageConfigHaddockHscolourCss = NoFlag,
          packageConfigHaddockContents = NoFlag,
          packageConfigHaddockIndex = NoFlag,
          packageConfigHaddockBaseUrl = NoFlag,
          packageConfigHaddockLib = NoFlag,
          packageConfigHaddockForHackage = NoFlag,
          packageConfigTestHumanLog = Flag "$pkgid-$test-suite.log",
          packageConfigTestMachineLog = Flag "$pkgid.log",
          packageConfigTestShowDetails = Flag Failures,
          packageConfigTestKeepTix = Flag False,
          packageConfigTestWrapper = NoFlag,
          packageConfigTestFailWhenNoTestSuites = Flag False,
          packageConfigTestTestOptions = [],
          packageConfigBenchmarkOptions = []
        },
    projectConfigLocalPackages =
      PackageConfig
        { packageConfigProgramPaths =
            MapLast {getMapLast = fromList []},
          packageConfigProgramArgs = MapMappend {getMapMappend = fromList []},
          packageConfigProgramPathExtra = ["/asd"],
          packageConfigFlagAssignment = fromList [],
          packageConfigVanillaLib = NoFlag,
          packageConfigSharedLib = NoFlag,
          packageConfigStaticLib = NoFlag,
          packageConfigDynExe = NoFlag,
          packageConfigFullyStaticExe = NoFlag,
          packageConfigProf = NoFlag,
          packageConfigProfLib = NoFlag,
          packageConfigProfExe = NoFlag,
          packageConfigProfDetail = NoFlag,
          packageConfigProfLibDetail = NoFlag,
          packageConfigConfigureArgs = [],
          packageConfigOptimization = NoFlag,
          packageConfigProgPrefix = NoFlag,
          packageConfigProgSuffix = NoFlag,
          packageConfigExtraLibDirs = [],
          packageConfigExtraLibDirsStatic = [],
          packageConfigExtraFrameworkDirs = [],
          packageConfigExtraIncludeDirs = [],
          packageConfigGHCiLib = NoFlag,
          packageConfigSplitSections = NoFlag,
          packageConfigSplitObjs = NoFlag,
          packageConfigStripExes = NoFlag,
          packageConfigStripLibs = NoFlag,
          packageConfigTests = NoFlag,
          packageConfigBenchmarks = NoFlag,
          packageConfigCoverage = NoFlag,
          packageConfigRelocatable = NoFlag,
          packageConfigDebugInfo = NoFlag,
          packageConfigDumpBuildInfo = NoFlag,
          packageConfigRunTests = NoFlag,
          packageConfigDocumentation = NoFlag,
          packageConfigHaddockHoogle = NoFlag,
          packageConfigHaddockHtml = NoFlag,
          packageConfigHaddockHtmlLocation = NoFlag,
          packageConfigHaddockForeignLibs = NoFlag,
          packageConfigHaddockExecutables = NoFlag,
          packageConfigHaddockTestSuites = NoFlag,
          packageConfigHaddockBenchmarks = NoFlag,
          packageConfigHaddockInternal = NoFlag,
          packageConfigHaddockCss = NoFlag,
          packageConfigHaddockLinkedSource = NoFlag,
          packageConfigHaddockQuickJump = NoFlag,
          packageConfigHaddockHscolourCss = NoFlag,
          packageConfigHaddockContents = NoFlag,
          packageConfigHaddockIndex = NoFlag,
          packageConfigHaddockBaseUrl = NoFlag,
          packageConfigHaddockLib = NoFlag,
          packageConfigHaddockForHackage = NoFlag,
          packageConfigTestHumanLog = NoFlag,
          packageConfigTestMachineLog = NoFlag,
          packageConfigTestShowDetails = NoFlag,
          packageConfigTestKeepTix = NoFlag,
          packageConfigTestWrapper = NoFlag,
          packageConfigTestFailWhenNoTestSuites = NoFlag,
          packageConfigTestTestOptions = [],
          packageConfigBenchmarkOptions = []
        },
    projectConfigSpecificPackage = MapMappend {getMapMappend = fromList []}
  }

As you can see, the "/asd" path ends up in the projectConfigAllPackages field and projectConfigLocalPackages field. However, the syncAndReadSourcePackagesRemoteRepos function that I'm fixing in this PR gets passed a projectConfigShared field, which only have "~/.cabal/bin".

Since I have no understanding of Cabal internals I stopped my analysis on that.

[Mikolaj]

Since I have no understanding of Cabal internals I stopped my analysis on that.

For almost all of us this is pure archaeology, so you are well enough equipped to reverse-engineer, document and fix, if you so choose. :)

[arrowd]

I lost a whole day on this "archeology" and don't plan to work on this anymore. Digging this brought me to the fact that cabal-install ignores some flags like --extra-prog-path for some reason and looking at the command line parsing code just made me freak out. The flag gets lost somewhere near cabal-install's Main.hs and when it reaches CmdConfigure.hs or any other command handler, there are no traces of it.

I was just passing by and wanted to fix a simple TODO. Instead I was sent down this rabbit hole to do "archeology". The code I touched got its fix, but it is not visible due to other (bigger) problems, which I'm not going to solve. I also don't want to spend any more time on this PR, so either merge it or close it.

[ulysses4ever]

@arrowd very helpful investigation, thank you! A little more inspection reveals that syncAndReadSourcePackagesRemoteRepos is only called from fetchAndReadSourcePackages, and that one is only called from two other places each of which has a handle on to a whole ProjectConfig (including projectConfigAllPackages and projectConfigLocalPackages). So, a quick and dirty solution to this problem would be to thread ProjectConfig instead of just projectConfigShared to syncAndReadSourcePackagesRemoteRepos (maybe you don’t need the whole thing but just projectConfigAllPackages or projectConfigLocalPackages would be enough: I haven’t checked).

I’m a bit slow to reply these days but I wish you all the luck on earth to deal with it anyway.

[ulysses4ever]

There are many other approaches one could take to solve it of course, above I only mention the easiest one. Another idea would be to look into how to set progPathExtra to something more interesting than the default. E.g. maybe --extra-prog-path could extend it too?

[arrowd]

syncAndReadSourcePackagesRemoteRepos is only called from fetchAndReadSourcePackages, and that one is only called from two other places each of which has a handle on to a whole ProjectConfig (including projectConfigAllPackages and projectConfigLocalPackages). So, a quick and dirty solution to this problem would be to thread ProjectConfig instead of just projectConfigShared to syncAndReadSourcePackagesRemoteRepos (maybe you don’t need the whole thing but just projectConfigAllPackages or projectConfigLocalPackages would be enough: I haven’t checked).

Yes, I considered this, but this is a hack. The syncAndReadSourcePackagesRemoteRepos should not require local project configs at all. Getting extraProgPath from local config us just another hack on top of existing hacks.

The one and only correct fix is to fix command line parsing.

[ulysses4ever]

5 years ago @gbaz commited f18b082 that changed the way ProjectConfigShared.projectConfigProgPathExtra is filled: it used to get its value from ConfigFlags.configProgramPathExtra (and that's what --extra-prog-path sets) but after that commit it now gets its value from GlobalFlags.globalProgPathExtra. The old behavior makes more sense to me: I'd like to be able to set that path per project. Incidentally, that's what would allow for easier testing (I think) and how @arrowd expected it to work. Luckily, we still have @gbaz around: maybe he could share why this change was made…

[gbaz]

Here's why the change was made: #4995 (comment)

and here's the PR that made it:

#4999

If you look at the summary of the changes in that PR, I don't thank that the above description matches what happened. (it just characterizes an intermediate unsquashed patch). Rather, it just sets globalProgPathExtra = projectConfigProgPathExtra. Before it didn't get it from anywhere?

I'm open to suggesting that command line flag should also be taken into account.

[ulysses4ever]

@gbaz

it just sets globalProgPathExtra = projectConfigProgPathExtra. Before it didn't get it from anywhere?

Yes, it did, that's the whole point of my previous comment: if you look at f18b082, it replaces

configProgramPathExtra    = projectConfigProgPathExtra

with

 globalProgPathExtra     = projectConfigProgPathExtra

and this change makes projectConfigProgPathExtra ignore the --extra-prog-path flag...

Now in this context, looking at #4999: what is --extra-prog-path-shared-only? Is it meant to be used instead of --extra-prog-path? Why the distinction? Should it be documented? Trying cabal configure --extra-prog-path-shared-only fails with unrecognized configure option...

[ulysses4ever]

@gbaz More generally, how do I set globalProgPathExtra from command-line? Does it have to be in the global config (~/.cabal/config)? How do you spell it there?

[gbaz]

The commit you are referencing is part of that larger PR. the thing it comments out was introduced incidentally in the course of that same PR. The PR as a whole does not introduce the behavior you suggest.

I don't remember this PR in detail, but extra-prog-path-shared-only is not a command line flag -- it looks like it was just introduced to make the roundtripping work, so it only works in a cabal.project file, and extra-prog-path as such should work in a global config file (i.e. ~/.cabal/config).

I think that the correct thing to do is just to change the existing --extra-prog-path flag to actually work in v2. My understanding is that before the change in that PR, neither that flag nor the cabal config way of setting it did anything much. After that PR, the cabal config way of setting it worked, but the command line way still did not.

[ulysses4ever]

The commit you are referencing is part of that larger PR. the thing it comments out was introduced incidentally in the course of that same PR. The PR as a whole does not introduce the behavior you suggest.

This is not true: just open the Files tab of #4999 and Ctrl-F for "DELETE ME" -- a mind-boggling comment left on the line where you commented out configProgramPathExtra = projectConfigProgPathExtra.

Not only this change made it into this PR, it's still in the code today, on master, check it out: https://github.com/haskell/cabal/blob/master/cabal-install/src/Distribution/Client/ProjectConfig/Legacy.hs#L503 (permalink).

I should say, I'm slightly surprised that we keep arguing about a thing that is so easy to check. f18b082 did made it into master and still there. As to whether it did made projectConfigProgPathExtra ignore --extra-prog-path or not, let me get back to you on that. It certainly looks like it to me.

[gbaz]

Yes, the line was commented out, and the commented out line remains.

But the line was introduced in 1a04a1a -- an earlier patch in the same PR.

So it is not that a line that existed before was commented out and replaced. It was a line that never existed, then was introduced, then commented out, in the course of working on the same PR.

Had I squashed the PR (which I now wish I had), then that commit would not exist in the history, and would not be confusing.

[ulysses4ever]

Oh, gosh, now I get it. Sorry for the noise!