Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Global configuration for project commands #4888

Closed
wants to merge 4 commits into from

Conversation

ttuegel
Copy link
Member

@ttuegel ttuegel commented Nov 13, 2017

Commits

Two deficiencies described in #3883 are corrected:

  1. Global configuration options are not correctly applied to all packages when using project commands.
  2. cabal.project offers no mechanism to specify project-wide global options.

Apply global configuration to all packages in project commands

The global configuration in ~/.cabal/config or CABAL_CONFIG should be applied to
all packages, even in the project (new-*) commands. Previously, package
configuration fields applied only to packages in the actual project. A new
field (projectConfigGlobalPackages) is added to ProjectConfig to track the
global package configuration.

See also: #3883 #4646

readGlobalConfig: Also consider cabal.config files in project root

If there is a cabal.config.local file in the project root directory,
readGlobalConfig reads it after ~/.cabal/config. This is intended to allow the
user to specify site-specific global options on a per-project basis. Global
options cannot be specified in cabal.project.local, which applies options only
to packages in the project.

See also: #3883 #4646

Checklist

  • 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.
  • If the change is docs-only, [ci skip] is used to avoid triggering the build bots.

Testing

I tested these changes by implementing #4646 in terms of global configuration files.

@23Skidoo
Copy link
Member

AppVeyor failure looks legit.

@23Skidoo
Copy link
Member

@hvr, does this conflict with your #4886?

@alexbiehl
Copy link
Member

This seems to be a more elaborated version of the idea in #4840. This definitely looks better. I will rebase the haddock stuff on this one I think.

@ttuegel
Copy link
Member Author

ttuegel commented Nov 14, 2017

AppVeyor failure looks legit.

The failures look related to changes in master since I started this branch. I will rebase.

@23Skidoo
Copy link
Member

/cc @dcoutts

The global configuration in ~/.cabal/config or CABAL_CONFIG should be applied to
all packages, even in the project (new-*) commands. Previously, package
configuration fields applied only to packages in the actual project. A new
field (projectConfigGlobalPackages) is added to ProjectConfig to track the
global package configuration.

See also: haskell#3883, haskell#4646
loadExactConfig will load just the config in the given file path or return
nothing if the file does not exist. loadRawConfig will continue to load the user
config in ~/.cabal/config or the file specified by CABAL_CONFIG. loadExactConfig
is factored out so that the same logic may be used to read config files from
other locations.
If there is a cabal.config.local file in the project root directory,
readGlobalConfig reads it after ~/.cabal/config. This is intended to allow the
user to specify site-specific global options on a per-project basis. Global
options cannot be specified in cabal.project.local, which applies options only
to packages in the project.

See also: haskell#3883 haskell#4646
The messages specifying the config file path source and which config files are
loaded had `notice' verbosity, but they are lowered to `debug' because it is not
likely that the user routinely needs this information.
@23Skidoo
Copy link
Member

@dcoutts promised to review this, so this is blocked on him for now.

@dcoutts
Copy link
Contributor

dcoutts commented Nov 22, 2017

I have some concerns about the approach. I'll get back with a proper review.

@hvr hvr requested a review from dcoutts November 25, 2017 17:29
@ttuegel
Copy link
Member Author

ttuegel commented Nov 28, 2017

I have some concerns about the approach.

@dcoutts
Does this mean you don't like how I have implemented this feature, or that you don't like the feature in the first place?

@ttuegel
Copy link
Member Author

ttuegel commented Dec 13, 2017

Since I haven't heard anything on this pull request in 3 weeks, I have had to find other ways to get cabal-install working on NixOS. Therefore, I am abandoning this PR in favor of tasks that are worth my time.

@ttuegel ttuegel closed this Dec 13, 2017
@ttuegel ttuegel deleted the site-config branch December 13, 2017 13:51
@23Skidoo
Copy link
Member

Sorry to hear this! @dcoutts, any chance you can find some time to look at this PR?

@dcoutts
Copy link
Contributor

dcoutts commented Dec 24, 2017

I'd like us to move in the direction of making config inclusions explicit, and this PR (though well motivated) is taking us further in the direction of implicit ad-hoc hard to understand/explain inclusion rules.

The global configuration in ~/.cabal/config or CABAL_CONFIG should be applied to all packages, even in the project (new-*) commands.

For example, if we want these kind of things then we should allow it to be explicit, e.g.

include-optional cabal.project.local
include-legacy-optional config.config.local $CABAL_CONFIG

A new field (projectConfigGlobalPackages) is added to ProjectConfig to track the global package configuration.

This is a good idea, but it needs corresponding syntax in the cabal.project files. I've taken this part of the patch and added syntax for it under the section name all-packages, see #4972.

If there is a cabal.config.local file in the project root directory, readGlobalConfig reads it after ~/.cabal/config.

I don't think we should be reading the legacy cabal.config by default. If we want it, lets be fully explicit about it, e.g. the include / include-legacy style idea above.

This is intended to allow the user to specify site-specific global options on a per-project basis. Global options cannot be specified in cabal.project.local, which applies options only to packages in the project.

They can now be specified in cabal.project.local files via the new syntax introduced in #4972. And with explicit includes the user could put those in any file they like (to manage manually).

@23Skidoo
Copy link
Member

I'm +1 on the idea of explicit includes, but I'd like to see the proposal spelled out in more detail.

include-optional cabal.project.local
include-legacy-optional config.config.local $CABAL_CONFIG

It's not very clear from this example what the proposed semantics of include-optional and include-legacy-optional should be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants