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

Create Cabal-syntax for .cabal parsing and types #7620

Merged
merged 1 commit into from
Jan 26, 2022

Conversation

patrickdoc
Copy link
Collaborator

@patrickdoc patrickdoc commented Sep 4, 2021


Please include the following checklist in your PR:

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


This PR splits Cabal into Cabal-syntax and Cabal (re: #7559).

The changes were generally conservative to avoid making design decisions. I mostly just pulled PackageDescription.hs and GenericPackageDescription.hs into the new package, and then moved code over as necessary to fix all the compilation errors.

The two code changes I did make were to move a few utility parsing functions so that Distribution.Verbosity stayed in Cabal rather than Cabal-syntax. Otherwise it was just import changes to avoid going through the Distribution.Simple.Utils module.

The result is split almost exactly in half (126 modules in Cabal-syntax, 121 modules in Cabal)

Comment on lines 75 to 93
Distribution.Compat.Binary
Distribution.Compat.DList
Distribution.Compat.Exception
Distribution.Compat.Lens
Distribution.Compat.Newtype
Distribution.Compat.NonEmptySet
Distribution.Compat.Prelude
Distribution.Compat.Semigroup
Copy link
Member

Choose a reason for hiding this comment

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

We may want to split the compat stuff between Cabal and Cabal-syntax, like it's done now with cabal-install and Cabal, but I'm not sure it's worth doing in this case.

Copy link
Collaborator

@phadej phadej Sep 4, 2021

Choose a reason for hiding this comment

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

I'd leave them for now, and just drop pre-GHC-8 support (#7531) before next major. That would cleanup plenty of stuff, leaving mostly "We rather not depend on dlist package and make our own copy".

The Distribution.Compat and Distribution.Utils are hard to separate. Lens is kind of utility, but otoh it's a compatibility shim because we cannot depend on lens 🤷

Copy link
Collaborator

@phadej phadej left a comment

Choose a reason for hiding this comment

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

This indeed looks very simple.

Could you make changes I mentioned.

Making CI pass will need some changes there, but I can take a look at that.

Note: GHC devs will need to be aware of this change, their build system will need adjustments too.

Distribution.Version
Language.Haskell.Extension

other-modules:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Most of these modules should be exposed-modules. In fact, the different lists could be combiled.

I don't think any module have to be hidden (i.e. other-module), except maybe some Compat modules, though I'm not sure about these either.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated

-- after checking if the file exists.
--
-- Argument order is chosen to encourage partial application.
readAndParseFile
Copy link
Collaborator

Choose a reason for hiding this comment

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

Moving these seems wrong. (probably related that this module have to exposed?)

Copy link
Collaborator Author

@patrickdoc patrickdoc Sep 4, 2021

Choose a reason for hiding this comment

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

So I moved these because "Verbosity" seems like a cabal-install specific thing as opposed to a Cabal-syntax thing, so I left it in Cabal.

These seem to be mostly for nice error reporting in a context where "Verbosity" makes sense, but I would guess most external use cases would rather handle the errors differently.

Edit: And the only internal use was in Distribution/Simple.hs, so it was easy enough to just pop over there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point about Verbosity.

@phadej
Copy link
Collaborator

phadej commented Sep 4, 2021

Most (maybe even all?) test-suites in Cabal can be moved to Cabal-syntax. These might highlight if things are in wrong place too (or imported from wrong place):

HackageTests.hs:import Distribution.Simple.Utils                   (fromUTF8BS, toUTF8BS)
UnitTests.hs:import Distribution.Simple.Utils
UnitTests.hs:import qualified UnitTests.Distribution.Simple.Glob
UnitTests.hs:import qualified UnitTests.Distribution.Simple.Program.GHC
UnitTests.hs:import qualified UnitTests.Distribution.Simple.Program.Internal
UnitTests.hs:import qualified UnitTests.Distribution.Simple.Utils
UnitTests.hs:    , testGroup "Distribution.Simple.Glob"
UnitTests.hs:        UnitTests.Distribution.Simple.Glob.tests
UnitTests.hs:    , UnitTests.Distribution.Simple.Program.GHC.tests
UnitTests.hs:    , testGroup "Distribution.Simple.Program.Internal"
UnitTests.hs:        UnitTests.Distribution.Simple.Program.Internal.tests
UnitTests.hs:    , testGroup "Distribution.Simple.Utils" $
UnitTests.hs:        UnitTests.Distribution.Simple.Utils.tests ghcPath
UnitTests/Distribution/Compat/Time.hs:import Distribution.Simple.Utils (withTempDirectory)
UnitTests/Distribution/Simple/Glob.hs:module UnitTests.Distribution.Simple.Glob
UnitTests/Distribution/Simple/Glob.hs:import Distribution.Simple.Glob
UnitTests/Distribution/Simple/Program/GHC.hs:module UnitTests.Distribution.Simple.Program.GHC (tests) where
UnitTests/Distribution/Simple/Program/GHC.hs:import Distribution.Simple.Program.GHC (normaliseGhcArgs)
UnitTests/Distribution/Simple/Program/GHC.hs:tests = testGroup "Distribution.Simple.Program.GHC"
UnitTests/Distribution/Simple/Program/Internal.hs:module UnitTests.Distribution.Simple.Program.Internal
UnitTests/Distribution/Simple/Program/Internal.hs:import Distribution.Simple.Program.Internal ( stripExtractVersion )
UnitTests/Distribution/Simple/Utils.hs:module UnitTests.Distribution.Simple.Utils
UnitTests/Distribution/Simple/Utils.hs:import Distribution.Simple.BuildPaths ( exeExtension )
UnitTests/Distribution/Simple/Utils.hs:import Distribution.Simple.Utils

Looks like very few unit tests are testing Distribution.Simple.

@phadej
Copy link
Collaborator

phadej commented Sep 4, 2021

Distribution.Simple.Glob could be renamed to Distribution.Utils.Glob.

@patrickdoc
Copy link
Collaborator Author

Running into some small compatibility issues, and not sure what the standard practices are here.

  1. I've locally moved the .cabal specific checks of this module: https://github.com/haskell/cabal/blob/master/Cabal/src/Distribution/PackageDescription/Check.hs to Cabal-syntax, but one of the checks requires autogenPathsModuleName (to verify Paths_Cabal, etc.). This is defined in Distribution.Simple.BuildPaths, and it doesn't seem reasonable to me to move all of those build paths into Cabal-syntax. Not sure if Paths_Cabal is an official part of the PackageDescription syntax, or if it's just an implementation detail of Cabal.

  2. Moving Distribution.Package to Cabal-syntax breaks hackage-security. https://github.com/haskell/hackage-security/search?p=1&q=Distribution It would be pretty trivial to move hackage-security to use Cabal-syntax, but not sure how these types of changes are usually handled. Otherwise I could add a compatibility layer, but there might be some weirdness with name conflicts.

@gbaz
Copy link
Collaborator

gbaz commented Sep 5, 2021

I'm not sure if moving the checks to cabal-syntax makes sense or not as a whole? I would thing the checks are for the semantics of a well-formed cabal file, not for parsing and manipulating its syntax. I can see arguments either way, though.

On the latter, could you use reexported modules? https://cabal.readthedocs.io/en/3.4/cabal-package.html#pkg-field-library-reexported-modules

I'm not sure if that fits in the support window for Cabal features, but I think it might?

@phadej
Copy link
Collaborator

phadej commented Sep 5, 2021

@gbaz, consider hackage-server. I think Cabal-syntax is enough for it, and hackage-server uses PD.Check. Also hackage-cli could use check. So i'd rather have that in Cabal-syntax, though I'm not against having Cabal-check lib too, but maybe that is too much at once!

@gbaz
Copy link
Collaborator

gbaz commented Sep 5, 2021

As the comments say:

-- This has code for checking for various problems in packages. There is one
-- set of checks that just looks at a 'PackageDescription' in isolation and
-- another set of checks that also looks at files in the package. Some of the

Hackage-server needs both types of checks, so I imagine it would need stuff that went beyond cabal-syntax regardless...

@phadej
Copy link
Collaborator

phadej commented Sep 5, 2021

So? The package content checking interface is abstracted:

-- | A record of operations needed to check the contents of packages.
-- Used by 'checkPackageContent'.
--
data CheckPackageContentOps m = CheckPackageContentOps {
    doesFileExist        :: FilePath -> m Bool,
    doesDirectoryExist   :: FilePath -> m Bool,
    getDirectoryContents :: FilePath -> m [FilePath],
    getFileContents      :: FilePath -> m BS.ByteString
  }

-- | Sanity check things that requires looking at files in the package.
-- This is a generalised version of 'checkPackageFiles' that can work in any
-- monad for which you can provide 'CheckPackageContentOps' operations.
--
-- The point of this extra generality is to allow doing checks in some virtual
-- file system, for example a tarball in memory.
--
checkPackageContent :: (Monad m, Applicative m)
                    => CheckPackageContentOps m
                    -> PackageDescription
                    -> m [PackageCheck]

Or put it other way around: hackage-server doesn't need to depend on Distribution.Simple, it doesn't build the packages. (builds do, but they probably use cabal-install or something higher level anyway!)

@gbaz
Copy link
Collaborator

gbaz commented Sep 6, 2021

Right, we could put all those checks into cabal syntax. My understanding is those checks do not belong in cabal-syntax, because they're beyond the scope of cabal-syntax, which is, as the name says, cabal syntax, and checks for well-formedness of a tarball don't make sense there. I'm proposing we put no checks in cabal syntax, since most clients will not need them, or those that will, will want all of them, including the ones that don't belong there.

As one of the primary people currently maintaining hackage-server, I really don't care if it needs to continue to depend on the whole Cabal library -- that seems fine to me.

@patrickdoc
Copy link
Collaborator Author

Reexported modules were exactly what I needed, thanks.

Looking at the check code again, there wasn't really a compelling reason to move it in the first place. Just a little over-aggressive in trimming the test dependencies. I'll roll that back.

@patrickdoc
Copy link
Collaborator Author

Rolled back the PackageDescription Check change, updated the rest of the modules, and I think CI should work now.

@@ -13,8 +13,9 @@ library
build-depends:
, base
, bytestring
, Cabal ^>=3.7.0.0
, QuickCheck ^>=2.13.2 || ^>=2.14
, Cabal ^>=3.7.0.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

What Cabal-QuickCheck and Cabal-described need from Cabal?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Cabal-QuickCheck needs:

  • Distribution.Simple.Compiler (DebugInfoLevel(..), OptimizationLevel(..), PackageDB(..),ProfDetailLevel(..),knownProfDetailLevels
  • Distribution.Simple.Flag (Flag(..))
  • Distribution.Simple.InstallDirs
  • Distribution.Simple.Setup (HaddockTarget(..), TestShowDetails(..))
  • Distribution.Utils.NubList
  • Distribution.Verbosity

Cabal-described needs Distribution.Verbosity

instance Described Verbosity where
    describe _ = REUnion
        [ REUnion ["0", "1", "2", "3"]
        , REUnion ["silent", "normal", "verbose", "debug", "deafening"]
          <> REMunch reEps (RESpaces <> "+" <>
            -- markoutput is left out on purpose
            REUnion ["callsite", "callstack", "nowrap", "timestamp", "stderr", "stdout" ])
        ]

@phadej
Copy link
Collaborator

phadej commented Sep 6, 2021

The

-- | The name of the auto-generated Paths_* module associated with a package
autogenPathsModuleName :: PackageDescription -> ModuleName
autogenPathsModuleName pkg_descr =
  ModuleName.fromString $
    "Paths_" ++ map fixchar (prettyShow (packageName pkg_descr))
  where fixchar '-' = '_'
        fixchar c   = c

can very well be in Cabal-syntax. #6452 is an issue which would make the name very much relevant for Cabal-syntax part (though parsing doesn't change).

@phadej
Copy link
Collaborator

phadej commented Sep 6, 2021

I tried this patch and it cabal-fmt and cabal-extras work with this patch by changing Cabal to Cabal-syntax.

Except of cabal-docspec which is expected, as it does cpp preprocessing itself, i.e. very much "builds" the packages.

@patrickdoc
Copy link
Collaborator Author

Updated artifact/bootstrap/meta CI, but not sure how to teach doctest about the new package.

@patrickdoc
Copy link
Collaborator Author

I think I've now fixed doctest and the Windows artifact issue, which just leaves the ghc 7.X failures due to reexported-modules not being supported.

@jneira
Copy link
Member

jneira commented Sep 7, 2021

Reexported modules were exactly what I needed, thanks.

Looking at the check code again, there wasn't really a compelling reason to move it in the first place. Just a little over-aggressive in trimming the test dependencies. I'll roll that back.

Other minor (?) caveat, reexported modules dont work well with stack repl see commercialhaskell/stack#5077 (comment)
If they dont work with stack repl the project could not be loaded in hls neither for now

Not sure if cabal can be built with stack, but it should to contribute to universal harmony 😛

@phadej
Copy link
Collaborator

phadej commented Sep 7, 2021

reexported-modules would also tightly tie Cabal-syntax and Cabal together: whenever there is a minor version of Cabal-syntax, there should be a minor version of Cabal too. That is probably not an issue for Cabal, but in general people have to be aware of that tradeoff. IMO Cabal don't need to re-export Cabal-syntax, though it would help in the migration, migration need is transient. (I expect very few projects having needs beyond Cabal-syntax).

@phadej
Copy link
Collaborator

phadej commented Sep 7, 2021

That said. It would be very nice to check what stack uses from Cabal. I.e. how much extra it needs beyond Cabal-syntax.

From my code-only scan, it seems it needs only macro generation (i.e. the same as cabal-docspec).

And now I'm starting to think where those belong. :(

@patrickdoc
Copy link
Collaborator Author

patrickdoc commented Sep 7, 2021

reexported-modules would also tightly tie Cabal-syntax and Cabal together: whenever there is a minor version of Cabal-syntax, there should be a minor version of Cabal too.

To be clear, I was only reexporting the single module Distribution.Package so that hacakge-security didn't need to be patched for cabal-install to build successfully. I can also update hackage-security if that route is easier.

That said. It would be very nice to check what stack uses from Cabal. I.e. how much extra it needs beyond Cabal-syntax.

From a quick look, the two most likely issues I saw were the Setup.hs files here: https://github.com/commercialhaskell/stack/search?q=Distribution.Simple

Edit: A slightly more rigorous search:

Cabal:
Distribution.PackageDescription.Check
Distribution.Simple.Build.Macros
Distribution.Verbosity

# unclear exactly what this is for, but it appears to not be more of a template than actual code
setup-shim/StackSetupShim.hs:
Distribution.Simple
Distribution.Simple.Build
Distribution.Simple.LocalBuildInfo
Distribution.Simple.Setup

cabal-install:
Distribution.Client.Upload

@phadej
Copy link
Collaborator

phadej commented Sep 8, 2021

Distribution.Package re-exports other stuff. Still, my argument holds, even single (small) re-exported module ties the packages together.

@ptkato ptkato mentioned this pull request Sep 8, 2021
3 tasks
@patrickdoc
Copy link
Collaborator Author

Any suggestions on how to deal with the breaking change for hackage-security without re-exporting Distribution.Package? It's needed for cabal-install, so we would need a patched version of hackage-security using the new Cabal-syntax for the builds to work.

@gbaz
Copy link
Collaborator

gbaz commented Sep 13, 2021

Just make a branch of hackage-security with the patch. Since its under the haskell org umbrella, we can make sure the patch is accepted when this is merged.

That said, I don't think reexported modules is problematic here. The concern about stack is about backpack, afaik, not reexported modules, which is a much earlier feature, and the version number coupling should not be a concern in our particular case. (i.e. the idea of an update to the syntax lib without an update to the main Cabal lib seems unlikely)

@jneira
Copy link
Member

jneira commented Sep 13, 2021

The concern about stack is about backpack, afaik, not reexported modules, which is a much earlier feature

yeah, i overlooked it, i hit the issue using mixins and no reexported-modules

@patrickdoc patrickdoc force-pushed the wip/cabal-syntax branch 3 times, most recently from ad61883 to 8bca501 Compare January 22, 2022 20:38
Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

LGTM

@Mikolaj
Copy link
Member

Mikolaj commented Jan 22, 2022

The hackage-security PR is merged, the dummy Cabal-syntax is on Hackage, all seems ready to go. Last moment to have a critical look before merge. This is quite a fabulous PR IMHO, but also very disruptive, so it'd be good not to revert and re-revert it too many times...

@phadej
Copy link
Collaborator

phadej commented Jan 22, 2022

I think this is great overall (and we might or might not move some modules around in the future). However, to avoid reverts, you should make GHC devs aware that this is going to happen soon, cause if they realise this is too late (in GHC release pipeline), there will be too much pressure to revert indeed.

@Mikolaj
Copy link
Member

Mikolaj commented Jan 22, 2022

I think this is great overall (and we might or might not move some modules around in the future). However, to avoid reverts, you should make GHC devs aware that this is going to happen soon, cause if they realise this is too late (in GHC release pipeline), there will be too much pressure to revert indeed.

Than you for the heads up. I've just told the GHC hackers and will liaison with them as needed.

Copy link
Member

@fgaz fgaz left a comment

Choose a reason for hiding this comment

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

Could you add Cabal-syntax in README.md, under "This Cabal Git repository contains the following packages:"?

Cabal/src/Distribution/Simple/PackageDescription.hs Outdated Show resolved Hide resolved
@patrickdoc
Copy link
Collaborator Author

Could you add Cabal-syntax in README.md, under "This Cabal Git repository contains the following packages:"?

Updated

@bgamari
Copy link
Contributor

bgamari commented Jan 23, 2022

I suspect that it shouldn't be too hard to adapt GHC to this change but I'll have to try to know for certain.

mtl >= 2.1 && < 2.3,
parsec >= 3.1.13.0 && < 3.2,
pretty >= 1.1.1 && < 1.2,
text >= 1.2.3.0 && < 1.3,
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be bumped to support text-2.0 as GHC has already bumped.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, we bumped this already on master, but mechanical rebasing did not reflect that in a PR a new package. I suppose the bounds need to be resynchronized with Cabal.cabal.

@Mikolaj
Copy link
Member

Mikolaj commented Jan 24, 2022

I suspect that it shouldn't be too hard to adapt GHC to this change but I'll have to try to know for certain.

Should we wait?

@bgamari
Copy link
Contributor

bgamari commented Jan 24, 2022

I suspect that it shouldn't be too hard to adapt GHC to this change but I'll have to try to know for certain.

Should we wait?

I am currently working on attempting the adaptation. So far so good but I will feel better once both build systems have passed the testsuite. I'll let you know shortly.

@bgamari
Copy link
Contributor

bgamari commented Jan 24, 2022

Is Cabal-syntax/src/Distribution/Fields/Lexer.hs guaranteed to be up-to-date?

@phadej
Copy link
Collaborator

phadej commented Jan 24, 2022

Is Cabal-syntax/src/Distribution/Fields/Lexer.hs guaranteed to be up-to-date?

Yes. Cabals own CI doesn't use alex either. And there is a check that it should be up to date.

(EDIT: And ghc-make/hadrian shouldn't rebuild it either).

@bgamari
Copy link
Contributor

bgamari commented Jan 24, 2022

I have confirmed that with ghc!7401 and the following patch I can bootstrap GHC:

diff --git a/Cabal-syntax/Cabal-syntax.cabal b/Cabal-syntax/Cabal-syntax.cabal
index 7649ac76b..91c04002d 100644
--- a/Cabal-syntax/Cabal-syntax.cabal
+++ b/Cabal-syntax/Cabal-syntax.cabal
@@ -41,7 +41,7 @@ library
     mtl        >= 2.1      && < 2.3,
     parsec     >= 3.1.13.0 && < 3.2,
     pretty     >= 1.1.1    && < 1.2,
-    text       >= 1.2.3.0  && < 1.3,
+    text       (>= 1.2.3.0  && < 1.3) || (>= 2.0 && < 2.1),
     time       >= 1.4.0.1  && < 1.12,
     -- transformers-0.4.0.0 doesn't have record syntax e.g. for Identity
     -- See also https://github.com/ekmett/transformers-compat/issues/35
diff --git a/ghc-packages b/ghc-packages
index 8d827f6f9..e9e369c42 100644
--- a/ghc-packages
+++ b/ghc-packages
@@ -1,2 +1,3 @@
 Cabal
+Cabal-syntax

@Mikolaj
Copy link
Member

Mikolaj commented Jan 24, 2022

Well spotted re /ghc-packages.

@patrickdoc
Copy link
Collaborator Author

Fixed text and ghc-packages + bumped time to < 1.13 to match Cabal.cabal

@Mikolaj
Copy link
Member

Mikolaj commented Jan 25, 2022

I think we are go. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Tell Mergify Bot to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants