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

Add 'all-packages' section to cabal.project files #4972

Closed
wants to merge 2 commits into from

Conversation

dcoutts
Copy link
Contributor

@dcoutts dcoutts commented Dec 24, 2017

Prior to this we had options that apply to all local packages, or options that apply to specific named packages. This new section allows all the same options but applies them to all packages local or not.

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

Please also shortly describe how you tested your change. Bonus points for added tests!

Prior to this we had options that apply to all local packages, or
options that apply to specific named packages. This new section allows
all the same options but applies them to all packages local or not.
@phadej
Copy link
Collaborator

phadej commented Dec 24, 2017

are options to "all local packages" specified on-top-level, or do they have own section?

@dcoutts
Copy link
Contributor Author

dcoutts commented Dec 24, 2017

are options to "all local packages" specified on-top-level, or do they have own section?

Yeah, top level. I was pondering adding an optional section, but then I'd have to think about what that redundancy means for the various round-trip properties.

@23Skidoo
Copy link
Member

Can you please add some documentation to the manual? The relevant section is https://github.com/haskell/cabal/blob/master/Cabal/doc/nix-local-build.rst#configuring-builds-with-cabalproject.

In the user guide, changelog and add a few code comments.
@dcoutts
Copy link
Contributor Author

dcoutts commented Dec 24, 2017

Can you please add some documentation to the manual

Done https://github.com/haskell/cabal/pull/4972/files#diff-f6c65c922526baba295563ecf772af73, plus changelog and code comments.

@phadej
Copy link
Collaborator

phadej commented Dec 24, 2017

@dcoutts does this solve #3579 ?

I.e we want be able to give ghc-options for local only packages. Not sure what's the state is after this PR

Copy link
Member

@23Skidoo 23Skidoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@dcoutts
Copy link
Contributor Author

dcoutts commented Dec 24, 2017

@dcoutts does this solve #3579 ?

No, but that's an easy fix. Should I add it to this PR too?

@23Skidoo opinions?

@23Skidoo
Copy link
Member

@dcoutts

What's your plan for #3579? Allowing top-level ghc-options?

@dcoutts
Copy link
Contributor Author

dcoutts commented Dec 24, 2017

What's your plan for #3579? Allowing top-level ghc-options?

Yeah, and also program locations. I can make it a separate PR.

@23Skidoo
Copy link
Member

I think we should also allow the program-default-options section if we're going to allow program-locations.

@23Skidoo
Copy link
Member

There's an AppVeyor failure that looks legit, but I haven't looked into it thoroughly.

@hvr
Copy link
Member

hvr commented Dec 25, 2017

How is precedence handled? Do more specific sections overwrite each other or do the combine monoidally (for list-like properties?)

If I wanted something like following:

  • Set optimization:2 to global packages only, except for
  • one specific global package glob-foo for which I have to force optimization:1,
  • while for local packages I want optimization:0, except for
  • a single local package loc-bar I want optimization:1

can I express this now?

PS: If all matching sections combine like monoids, we may run into expressibility issues for list-like properties (c.f. similiar issue in common blocks and the !-idea)

@23Skidoo
Copy link
Member

I've restarted the AppVeyor build, and it's green now. If the failure happens again, we'll need to look into it in more thoroughly.

@dcoutts
Copy link
Contributor Author

dcoutts commented Dec 25, 2017

How is precedence handled? Do more specific sections overwrite each other or do the combine monoidally (for list-like properties?)

Yes, monoidal, just like local + specific. This makes it all + local + specific.

If I wanted something like following:

Set optimization:2 to global packages only, except for
one specific global package glob-foo for which I have to force optimization:1,
while for local packages I want optimization:0, except for
a single local package loc-bar I want optimization:1

can I express this now?

Yes:

all-packages
  optimisation: 2

optimisation: 0

package foo
  optimisation: 1
package bar
  optimisation: 1

PS: If all matching sections combine like monoids, we may run into expressibility issues for list-like properties (c.f. similiar issue in common blocks and the !-idea)

Yes. That general problem still exists. This doesn't make it better or worse.

@hvr
Copy link
Member

hvr commented Dec 25, 2017

@dcoutts that sounds great; I only have a minor bikeshedding concern/question: why not use a more "uniform" syntax? Say we wanted to extend the syntax to allow globbing, like e.g.

package amazonka-*
   ...

package {foo,bar}
  ...

wouldn't it be more uniform if all-packages was instead simply

packages *
   ...

?

@23Skidoo
Copy link
Member

@dcoutts IIUC, the problem described in #3579 will still exist with this solution, because

package Cabal
    ghc-options: -foo

doesn't allow to distinguish between local package Cabal and lib:Cabal used to build setup scripts of dependencies. Admittedly a small problem, though, since only lib:Cabal is special in this way, and you can do something like

$ cabal new-build --only-dep
$ $EDITOR cabal.project
# Enable ghc-options: -foo for package Cabal
$ cabal new-build

@dcoutts
Copy link
Contributor Author

dcoutts commented Dec 27, 2017

@hvr I was worried someone might suggest that. :-) But now that I've thought about it for a bit, I think it's not that hard. Let me have a go.

@hvr
Copy link
Member

hvr commented Dec 27, 2017

@dcoutts Just something to take into account: in future we may also need to be apply/inject options at a per-component granularity (for packages which are per-component capable at least); e.g. you may want to inject ghc-options: -Werror only to to the library component; but maybe for that we should allocate a new keyword component, e.g.

component haddock-library:lib:haddock-library
   ghc-options: -Werror

component haddock-library:lib:attoparsec
   optimization: 2

component haddock-library:tests
   ghc-options: -Wno-error

or something like that, where component understands a big part of the target grammar... or actually even better yet, nested/hierarchical:

package haddock-library
  component lib:haddock-library
     ghc-options: -Werror

  component lib:attoparsec
     optimization: 2

  component tests
     ghc-options: -Wno-error

Just wanted to write this down somewhere... so it doesn't get lost... I hope I haven't increased your worry-level :-)

@dcoutts
Copy link
Contributor Author

dcoutts commented Dec 28, 2017

Yeah, once you start thinking about it you realise that there's probably a whole expression language for selecting which packages/components options should apply to and under what circumstances (e.g. different named configurations, perhaps like flags in .cabal files). Remember that ultimately I want us to be able to have multiple different configurations active at once, e.g. building frontend & backend together with ghc & ghcjs.

So one wrinkle I've already discovered is that solver options cannot so easily be applied per-package in a style in which these sections are PackageName -> Bool style predicates (like matching the glob "amazonka-*"), rather than a specific Set PackageName. This is because the interface to the solver is (quite reasonably) in terms of listed constraints on packages, and not predicate functions.

However, if we can impose the restriction that per-package solver flags can only be listed in sections with "enumerable" globs, and not wildcard globs then we can make it work, e.g.

Allowed:

package foo bar {foo,bar}-{baz}
  flags: +whizzle -flibbit

Not allowed:

package amazonka-*
  enable-tests: True
  enable-benchmarks: False

Sound acceptable?

@23Skidoo
Copy link
Member

Can't we expand wildcard globs after reading in the package index, but before running the solver?

@dcoutts
Copy link
Contributor Author

dcoutts commented Dec 28, 2017

Can't we expand wildcard globs after reading in the package index, but before running the solver?

Perhaps so now. Previously we tried hard to avoid looking at most packages, but I guess this is just based on package name so the index/cache will handle it. I would need to make the glob matching more efficient however to be able to run in on all the package names.

@23Skidoo
Copy link
Member

Adding some kind of trie-based structure for looking up package names may be required.

@hvr
Copy link
Member

hvr commented Dec 28, 2017

I'd just want to point out that I don't expect for us to implement proper globbing/{}-expansion right now if it's non-trivial; If we can agree that package <some-glob-ish-grammar> is the right direction, then let's just support the special package * case! :-)

@23Skidoo
Copy link
Member

23Skidoo commented Jan 3, 2018

Merged manually, see ab585e8.

@23Skidoo 23Skidoo closed this Jan 3, 2018
@23Skidoo 23Skidoo deleted the dcoutts/project-config-explicit-includes branch January 3, 2018 15:00
@angerman
Copy link
Collaborator

#5053 implements @hvr's package * suggestion.

@angerman angerman mentioned this pull request Jan 19, 2018
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants