Warn if builddir is going to be ignored

[andreabedini]

Closes: #7941

---

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.
• The documentation has been updated, if necessary.
• Include manual QA notes if your PR relates to cabal-install.

Bonus points for added automated tests!

[ulysses4ever]

Terrific! I'll take a look later but in the meantime: does it also address #8947?

[andreabedini]

@ulysses4ever This PR addresses #7941 that is: builddir is ignored without a warning. It does not address or attempts to solve #8947 & #5271 since that is a more complex issue.

Note to myself. I am only warning about builddir beign not supported in project files but it seems also not supported in the global config? I need to double check.

[ulysses4ever]

I was unclear, i guess, when i referenced #8947. I'm asking to check if this warning will be printed when builddir comes not from a project file (which was your original motivation, afaiu) but from the global config. In my mind, this is very likely, but maybe I'm utterly wrong.

[andreabedini]

I was unclear, i guess, when i referenced #8947. I'm asking to check if this warning will be printed when builddir comes not from a project file (which was your original motivation, afaiu) but from the global config.

It was indeed my original motivation but on a second reading I realised builddir is ignored also from the global config. As it stands this change will not warn on the global config, because readGlobalConfig calls the old loadConfig rather than readProjectFileSkeleton.

I can make the warning appear irrespectively from where buildir is set by moving the check. I will do that.

[andreabedini]

@ulysses4ever I have corrected the manual too. As far as I can tell everytime the manual says "This option cannot be specified via a cabal.project file" it means the global config file too, effectively meaning "can only be specified from the command line".

[ulysses4ever]

Terrific!

[ulysses4ever]

As far as I can tell everytime the manual says "This option cannot be specified via a cabal.project file" it means the global config file too, effectively meaning "can only be specified from the command line".

Maybe we should update the other places like that (to say "only command line"). I think it'd read much clearer.

[andreabedini]

Maybe we should update the other places like that (to say "only command line"). I think it'd read much clearer.

If you agree with my interpretation, yes, I can definitely do that. This is how I think it works:

• All configuration is passed around in the form of ProjectConfig. When global configuration is loaded (using some legacy parsing), it is converted into a ProjectConfig (convertLegacyGlobalConfig), cli arguments are converted to ProjectConfig (commandLineFlagsToProjectConfig), and the project files are read directly into ProjectConfig (in two steps because of conditionals: readProjectConfig and instantiateProjectConfigSkeletonWithCompiler).
• Note: The is no other configuration other than ProjectConfig!
• All new commands start with establishProjectBaseContext which is passed the configuration obtained from the command line arguments (cliConfig).
• The code path then looks like this

establishProjectBaseContext verbosity cliConfig currentCommand = do
    projectRoot <- either throwIO return =<< findProjectRoot verbosity mprojectDir mprojectFile
    establishProjectBaseContextWithRoot verbosity cliConfig projectRoot currentCommand
  where
    mprojectDir    = Setup.flagToMaybe projectConfigProjectDir
    mprojectFile   = Setup.flagToMaybe projectConfigProjectFile
    ProjectConfigShared { projectConfigProjectDir, projectConfigProjectFile } = projectConfigShared cliConfig

-- which calls

establishProjectBaseContextWithRoot verbosity cliConfig projectRoot currentCommand = do
    let haddockOutputDir = flagToMaybe (packageConfigHaddockOutputDir (projectConfigLocalPackages cliConfig))
    let distDirLayout = defaultDistDirLayout projectRoot mdistDirectory haddockOutputDir

    httpTransport <- configureTransport verbosity
                     (fromNubList . projectConfigProgPathExtra $ projectConfigShared cliConfig)
                     (flagToMaybe . projectConfigHttpTransport $ projectConfigBuildOnly cliConfig)

    (projectConfig, localPackages) <-
      rebuildProjectConfig verbosity
                           httpTransport
                           distDirLayout
                           cliConfig
    ...

-- which calls

rebuildProjectConfig verbosity httpTransport DistDirLayout {...} cliConfig = do
    ...
    (projectConfig, localPackages) <-
      runRebuild distProjectRootDirectory
      $ ...
      $ do
          ...
          projectConfig <- instantiateProjectConfigSkeletonFetchingCompiler ...
          localPackages <- phaseReadLocalPackages (projectConfig <> cliConfig)
          return (projectConfig, localPackages)
    ...
    return (projectConfig <> cliConfig, localPackages)

• You can see that: projectConfigProjectDir, projectConfigProjectFile, packageConfigHaddockOutputDir, projectConfigProgPathExtra, projectConfigHttpTransport are effective only when set through the command line (cliConfig).
• All the configurations are eventually merged into a single ProjectConfig but only after the above settings are used.

If we want to be able to read those settings from a global configuration file, we would need to move them out from ProjectConfig to a separate object which can be read at the very start; and somehow incorporate these "initial settings" with the other "project settings" in the global configuration file. But I'd suggest we do that only after we finish the transition to the new parser (see #6101 and #8889).

@ulysses4ever If you agree with the above analysis, I will change what the manual says about the above mentioned options (projectConfigProjectDir, projectConfigProjectFile, packageConfigHaddockOutputDir, projectConfigProgPathExtra and projectConfigHttpTransport)

[ulysses4ever]

@andreabedini this is an astonishingly clear description, thank you! It matches what I was able to discern myself about project config. I am in total support of improving the manual as discussed.

I also think your description would be nice to put somewhere. Either comments in some source file (e.g. ProjectOrchestration.hs) or the wiki here. The former probably has a much higher chance to be read by someone. It may be a sort of a GHC Note in the source...

[andreabedini]

@ulysses4ever Done. I added a note about the interaction between --http-transport and --extra-prog-path. I decided to leave --haddock-output-dir alone because I am not sure about it. It seems to be fixed in the distDirLayout but then it's also used later on.

[ulysses4ever]

Andrea, while we're on it (the famous last words...), what do you think about defaulting packages to the current directory even if a project file is present? This has been requested numerous times. At least, #8679 and #5850. I think it's pretty clear that having the same default, whether you have a project file or not, is a better UX. Maybe we could rip this band aid in this pr too?

[ulysses4ever]

@andreasabel since this PR addresses the issue you opened (#7941), would you mind giving it a review if time allows?

[andreasabel]

Great, thanks!

[andreabedini]

Andrea, while we're on it (the famous last words...), what do you think about defaulting packages to the current directory even if a project file is present? This has been requested numerous times. At least, #8679 and #5850. I think it's pretty clear that having the same default, whether you have a project file or not, is a better UX. Maybe we could rip this band aid in this pr too?

I suggest we leave that for another PR. I commented on that thread.

[ulysses4ever]

Sounds good, thank you. Please, feel free to apply the squash+merge label as soon as you feel comfortable.