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

Omit false conds #404

Merged
merged 2 commits into from
Nov 4, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,25 @@ at the Singapore Haskell meetup: http://typeful.net/talks/hpack

## Documentation

### Handling of `Paths_` modules

Cabal generates a `Paths_` module for every package. By default Hpack adds
that module to `other-modules` when generating a `.cabal` file. This is
sometimes useful and most of the time not harmful.

However, there are situations when this can lead to compilation errors (e.g
when using a custom `Prelude`).

To prevent Hpack from adding the `Paths_` module to `other-modules` add the
following to `package.yaml`:

```yaml
library:
when:
- condition: false
other-modules: Paths_name # substitute name with the package name
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we include a note here about how Cabal generates these modules names? I think it replaces hyphens with underscores. I'm not sure if it does anything else.

Copy link
Owner Author

Choose a reason for hiding this comment

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

That's the only thing I'm aware of. Ideally, I would like to link to the Cabal docs on Path_, however, I couldn't find anything.

@phadej do you know if the Cabal docs cover Paths_ anywhere?

```

### Quick-reference

#### Top-level fields
Expand Down Expand Up @@ -376,7 +395,6 @@ Conditionals with an else branch:
- Must have a `condition` field
- Must have a `then` field, itself an object containing any number of other fields
- Must have a `else` field, itself an object containing any number of other fields
- All other top-level fields are ignored

For example,

Expand All @@ -394,6 +412,8 @@ becomes
else
ghc-options: -O0

**Note:** Conditionals with `condition: false` are omitted from the generated
`.cabal` file.
Copy link
Owner Author

Choose a reason for hiding this comment

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

@tfausak do you see any potential issues with this behavior. One reason why I'm eager to do this is that it will make the fix for #400 less convoluted.

Copy link
Owner Author

Choose a reason for hiding this comment

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

It also gives us cleaner .cabal files, which doesn't hurt.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems safe to me!


### <a name="file-globbing"></a>File globbing

Expand Down
24 changes: 15 additions & 9 deletions src/Hpack/Config.hs
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ module Hpack.Config (
, Library(..)
, Executable(..)
, Conditional(..)
, Cond(..)
, Flag(..)
, SourceRepository(..)
, BuildType(..)
Expand All @@ -70,7 +71,6 @@ module Hpack.Config (
, renameDependencies
, Empty(..)
, pathsModuleFromPackageName
, Cond(..)

, LibrarySection(..)
, fromLibrarySectionInConditional
Expand Down Expand Up @@ -468,17 +468,16 @@ hasKey key (Object o) = HashMap.member key o
hasKey _ _ = False

newtype Condition = Condition {
_conditionCondition :: Cond
conditionCondition :: Cond
} deriving (Eq, Show, Generic, FromValue)

newtype Cond = Cond String
data Cond = CondBool Bool | CondExpression String
deriving (Eq, Show)

instance FromValue Cond where
fromValue v = case v of
String s -> return (Cond $ T.unpack s)
Bool True -> return (Cond "true")
Bool False -> return (Cond "false")
String c -> return (CondExpression $ T.unpack c)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this means someone could avoid this new filtering behavior by explicitly passing a string. Like this:

when:
  - condition: 'false'

That's probably fine, and may in fact be desired as a workaround.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, exactly, that's what I thought.

Bool c -> return (CondBool c)
_ -> typeMismatch "Boolean or String" v

data ThenElse cSources cxxSources jsSources a = ThenElse {
Expand Down Expand Up @@ -955,7 +954,7 @@ data Section a = Section {
} deriving (Eq, Show, Functor, Foldable, Traversable)

data Conditional a = Conditional {
conditionalCondition :: String
conditionalCondition :: Cond
, conditionalThen :: a
, conditionalElse :: Maybe a
} deriving (Eq, Show, Functor, Foldable, Traversable)
Expand Down Expand Up @@ -1277,6 +1276,13 @@ getMentionedLibraryModules (LibrarySection _ _ exposedModules generatedExposedMo
listModules :: FilePath -> Section a -> IO [Module]
listModules dir Section{..} = concat <$> mapM (getModules dir) sectionSourceDirs

removeConditionalsThatAreAlwaysFalse :: Section a -> Section a
removeConditionalsThatAreAlwaysFalse sect = sect {
sectionConditionals = filter p $ sectionConditionals sect
}
where
p = (/= CondBool False) . conditionalCondition

inferModules ::
FilePath
-> String
Expand All @@ -1286,7 +1292,7 @@ inferModules ::
-> ([Module] -> a -> b)
-> Section a
-> IO (Section b)
inferModules dir packageName_ getMentionedModules getInferredModules fromData fromConditionals = traverseSectionAndConditionals
inferModules dir packageName_ getMentionedModules getInferredModules fromData fromConditionals = fmap removeConditionalsThatAreAlwaysFalse . traverseSectionAndConditionals
(fromConfigSection fromData [pathsModuleFromPackageName packageName_])
(fromConfigSection (\ [] -> fromConditionals) [])
[]
Expand Down Expand Up @@ -1427,7 +1433,7 @@ toSection packageName_ executableNames = go
ThenElseConditional (Product (ThenElse then_ else_) c) -> conditional c <$> (go then_) <*> (Just <$> go else_)
FlatConditional (Product sect c) -> conditional c <$> (go sect) <*> pure Nothing
where
conditional (Condition (Cond c)) = Conditional c
conditional = Conditional . conditionCondition

type SystemBuildTool = (String, VersionConstraint)

Expand Down
8 changes: 7 additions & 1 deletion src/Hpack/Render.hs
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,13 @@ renderConditional renderSectionData (Conditional condition sect mElse) = case mE
Nothing -> if_
Just else_ -> Group if_ (Stanza "else" $ renderSection renderSectionData [] [] else_)
where
if_ = Stanza ("if " ++ condition) (renderSection renderSectionData [] [] sect)
if_ = Stanza ("if " ++ renderCond condition) (renderSection renderSectionData [] [] sect)

renderCond :: Cond -> String
renderCond = \ case
CondExpression c -> c
CondBool True -> "true"
CondBool False -> "false"

defaultLanguage :: Element
defaultLanguage = Field "default-language" "Haskell2010"
Expand Down
19 changes: 19 additions & 0 deletions test/EndToEndSpec.hs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,25 @@ spec = around_ (inTempDirectoryNamed "foo") $ do
"package.yaml: Duplicate field $.name"
]

describe "handling of Paths_ module" $ do
it "adds Paths_ to other-modules" $ do
[i|
library: {}
|] `shouldRenderTo` library [i|
other-modules:
Paths_foo
|]

context "when Paths_ is mentioned in a conditional that is always false" $ do
it "does not add Paths_" $ do
[i|
library:
when:
- condition: false
other-modules: Paths_foo
|] `shouldRenderTo` library [i|
|]

describe "spec-version" $ do
it "accepts spec-version" $ do
[i|
Expand Down
7 changes: 7 additions & 0 deletions test/Helper.hs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
{-# LANGUAGE FlexibleContexts #-}
{-# LANGUAGE ConstraintKinds #-}
{-# OPTIONS_GHC -fno-warn-orphans #-}
module Helper (
module Test.Hspec
, module Test.Mockery.Directory
Expand All @@ -13,6 +14,7 @@ module Helper (

import Test.Hspec
import Test.Mockery.Directory
import Data.String
import Control.Monad
import Control.Applicative
import System.Directory (getCurrentDirectory, setCurrentDirectory, canonicalizePath)
Expand All @@ -23,6 +25,11 @@ import System.FilePath
import Data.Yaml.TH (yamlQQ)
import Language.Haskell.TH.Quote (QuasiQuoter)

import Hpack.Config

instance IsString Cond where
fromString = CondExpression

withCurrentDirectory :: FilePath -> IO a -> IO a
withCurrentDirectory dir action = do
bracket getCurrentDirectory setCurrentDirectory $ \ _ -> do
Expand Down
6 changes: 3 additions & 3 deletions test/Hpack/ConfigSpec.hs
Original file line number Diff line number Diff line change
Expand Up @@ -643,17 +643,17 @@ spec = do
it "accepts Strings" $ do
[yaml|
os(windows)
|] `shouldDecodeTo_` Cond "os(windows)"
|] `shouldDecodeTo_` CondExpression "os(windows)"

it "accepts True" $ do
[yaml|
yes
|] `shouldDecodeTo_` Cond "true"
|] `shouldDecodeTo_` CondBool True

it "accepts False" $ do
[yaml|
no
|] `shouldDecodeTo_` Cond "false"
|] `shouldDecodeTo_` CondBool False

it "rejects other values" $ do
[yaml|
Expand Down