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

[sjakobi]

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;

• import statements have to be the first thing in a stanza haskell/cabal#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:

hackage-cli/src/Main.hs

Lines 780 to 822 in fbf1967

	AddBound AddBoundOptions {..} -> forM_ optABFiles $ \fp -> do
	old <- BS.readFile fp
	-- idea is simple:
	-- - .cabal is line oriented file
	-- - find "library" section start
	-- - bonus: look of an indentation used from the next field/section there
	-- - insert data into a bytestring "manually"
	fs <- either (exitFailureWith . show) return $ C.readFields old
	(lin, indent) <- maybe
	(exitFailureWith $ "Cannot find library section in " ++ fp)
	return
	(findLibrarySection fs)
	let msgLines = map ("-- " ++) optABMessage
	bdLine = "build-depends: " ++ C.prettyShow optABPackageName ++ " " ++ C.prettyShow optABVersionRange
	midLines = [ BS8.pack $ replicate indent ' ' ++ l
	| l <- msgLines ++ [bdLine]
	] ++ [""] -- also add an empty line separator
	(preLines, postLines) = splitAt lin $ BS8.lines old
	new = BS8.unlines (preLines ++ midLines ++ postLines)
	-- sanity check
	let oldGpd = parseGenericPackageDescription' old
	newGpd = parseGenericPackageDescription' new
	oldRange = extractRange oldGpd optABPackageName
	newRange = extractRange newGpd optABPackageName
	oldRange' = C.intersectVersionRanges oldRange optABVersionRange
	unless (C.toVersionIntervals newRange == C.toVersionIntervals oldRange') $
	exitFailureWith $ unwords
	[ "Edit failed, version ranges don't match: "
	, C.prettyShow oldRange
	, "&&"
	, C.prettyShow optABVersionRange
	, "=/="
	, C.prettyShow newRange
	]
	-- write new version
	BS.writeFile fp new

…and the function that finds the insertion position:

hackage-cli/src/Main.hs

Lines 441 to 449 in fbf1967

	findLibrarySection :: [C.Field C.Position] -> Maybe (Int, Int)
	findLibrarySection [] = Nothing
	findLibrarySection (C.Section (C.Name (C.Position row _) "library") [] fs : _) =
	Just (row, findIndent fs)
	where
	findIndent [] = 4
	findIndent (f : _) = case C.fieldAnn f of
	C.Position _ col -> pred col
	findLibrarySection (_ : fs) = findLibrarySection fs

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

[Bodigrim]

I don’t want it to look as a shameless plug, but how do we feel about making hackage-cli to use cabal-add API instead of manual updates? cabal-add is almost two years old now and I have not received any complaints about how it works.

[andreasabel]

I wouldn't object a reimplementation of the add-bound command.

Personally, I stopped using it, I am using wgrep in Emacs to do bulk-editing of cabal files, with much better control over the result than with add-bound.