Adding bounds to library with common stanza results in malformed cabal file

[sjakobi]

trafficstars

Given https://hackage.haskell.org/package/ede-0.3.2.0/revision/0.cabal, if I run

$ hackage-cli add-bound aeson '< 1.6' ede-0.3.2.0.cabal

, hackage-cli applies the following patch:

@@ -56,6 +56,8 @@ common base
   other-modules:    Paths_ede
 
 library
+  build-depends: aeson <1.6
+
   import:          base
   hs-source-dirs:  lib
   exposed-modules:

If I try to push this revision, the response is:

Warning: ede-0.3.2.0.cabal:61:3: Unknown field: "import"
Pushing "ede-0.3.2.0.cabal" (ede-0.3.2.0~0) [review-mode] ...
Hackage response was (after 0.622 secs):
================================================================================
Errors:

Cannot remove existing library dependency on 'base' in library  component

================================================================================

cabal check reveals:

Warning: These warnings may cause trouble when distributing the package:
Warning: ede.cabal:61:3: Unknown field: import. Common stanza imports should
be at the top of the enclosing section
Warning: Hackage would reject this package.

[andreasabel]

cabal check says:

Common stanza imports should be at the top of the enclosing section

I'd say this is also an upstream problem;

• https://github.com/haskell/cabal/issues/5564

it is questionable for a declarative language that cabal aims to be to demand a certain ordering of the fields. A reason to demand import to be at the top would be that it brings identifiers into scope that are used subsequently. But is this the case for a stanza import?

[sjakobi]

I agree that it is a surprising constraint. I have no idea about the reasons though.

[phadej]

The way .cabal file works (due decisions done long time ago)

common foo
  something: foo
  if flag(foo-flag)
    something: foo1
  else
    something: foo2

library
  header: headerVal
  if os(windows)
    something: windows

  import: common

  footer: footerVal
  if impl(ghc)
    something: ghc

would desugar into

library
  header: headerVal
  if os(windows)
    something: windows

  something: foo
  if flag(foo-flag)
    something: foo1
  else
    something: foo2

  footer: footerVal
  if impl(ghc)
    something: ghc

and interpret as if it were

library
  header: headerVal
  something: foo
  footer: footerVal

  if os(windows)
    something: windows

  if flag(foo-flag)
    something: foo1
  else
    something: foo2

  if impl(ghc)
    something: ghc

because conditionals are pushed back.

That would be fine if all fields contents were commutative (like build-depends), but buildable isn't, for example, the last one wins.

Thus to avoid any confusion imports have to come first.

That was also a conservative change, it can be relaxed later. It would be better to "fix" the push-back of conditionals first though.

---

That said, future relaxations won't help hackage-cli, as older spec cabal files still have to be dealt with.

[sjakobi]

To fix the issue, I think we can simply tweak the existing logic so the additional build-depends are inserted after any imports.

Here's the add-bound implementation:

https://github.com/hackage-trustees/hackage-cli/blob/fbf1967b49a352719b6e2b094b72fd76f01c27a2/src/Main.hs#L780-L822

…and the function that finds the insertion position:

https://github.com/hackage-trustees/hackage-cli/blob/fbf1967b49a352719b6e2b094b72fd76f01c27a2/src/Main.hs#L441-L449

[Bodigrim]

I've developed https://github.com/Bodigrim/cabal-add, capable to insert dependencies even in the presense of common stanzas.

[andreasabel]

@Bodigrim Kudos! I'll try it out next time I need to make revisions...