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

Parse failure for 'if' with flags inside test-suite #5055

Closed
chshersh opened this issue Jan 18, 2018 · 3 comments
Closed

Parse failure for 'if' with flags inside test-suite #5055

chshersh opened this issue Jan 18, 2018 · 3 comments
Assignees
Milestone

Comments

@chshersh
Copy link
Member

I'm following these suggestions from official documentation to use different main files for different flags:

Real problem: I need to disable doctest tests for Windows because AppVeyor CI fails

But when I'm trying to use this approach for my test-suite I see parsing error when using cabal new-build command.

Expected behavior

Build without problems

Observed behavior

$ cabal new-build
Warning:
.../cabal-flag-example/cabal-flag-example.cabal:11:1:
The 'main-is' field is required for the exitcode-stdio-1.0 test suite type.
cabal: Failed parsing
".../cabal-flag-example/./cabal-flag-example.cabal".

I've tried to create minimal reproducible example. You can see it here:

$ cabal --version
cabal-install version 2.1.0.0
compiled using version 2.1.0.0 of the Cabal library 
@phadej
Copy link
Collaborator

phadej commented Jan 18, 2018

So far I think this is a documentation bug

Trying with cabal-install-2.0 gives:

% /opt/cabal/2.0/bin/cabal check
cabal: ./cabal-flag-example.cabal:11: The 'main-is' field is required for the
exitcode-stdio-1.0 test suite type.

For now you can workaround this by writing (conditional statements are interpreted after "main" body):

test-suite flag-cabal-test
  type:                exitcode-stdio-1.0
  build-depends:       base >= 4.8 && < 5
  default-language:    Haskell2010

  main-is:             SecondMain.hs

  if os(windows)
    main-is:             FirstMain.hs

or having dummy Main.hs which includes module based on hs-source-dirs:

module Main (main) where
import Tests (main)

and

test-suite flag-cabal-test
  type:                exitcode-stdio-1.0
  build-depends:       base >= 4.8 && < 5
  default-language:    Haskell2010

  main-is: Main.hs

  hs-source-dirs: common
  other-modules: Tests

  if os(windows)
    hs-source-dirs: windows
  else 
    hs-source-dirs: unix

Currently we are conservative when checking that test-suite always have type, and if it's exitcode-stdio-1.0 then also main-is (for detailed-0.9 we required test-module). To fix this we need to verify that all possible "flattenings" have type, and then either main-is or test-module depending on the type. More concretely we need to generalise validateTestSuite check into validateCondTreeTestSuite.

Now as I look into the code, that check looks fishy anyway as

test-suite flag-cabal-test
  build-depends:       base >= 4.8 && < 5
  default-language:    Haskell2010

  if os(windows)
    type:                exitcode-stdio-1.0
    main-is:             FirstMain.hs
  else
    type:                exitcode-stdio-1.0
    main-is:             SecondMain.hs

works

AND with current-ish master also

test-suite flag-cabal-test
  build-depends:       base >= 4.8 && < 5
  default-language:    Haskell2010

  if os(windows)
    type:                exitcode-stdio-1.0
    main-is:             FirstMain.hs

which should fail and does fail with cabal-install-2.0:

cabal: ./cabal-flag-example.cabal:11: Test suite "flag-cabal-test" is missing
required field "type" or the field is not present in all conditional branches.
The available test types are: exitcode-stdio-1.0, detailed-0.9

that said, I think that fix won't be invasive. May need to change the public interface of
Distribution.PackageDescription.FieldGrammar. @23Skidoo is it ok to do the patch next week, to be still included in 2.2?

@phadej phadej added this to the 2.2 milestone Jan 18, 2018
@chshersh
Copy link
Member Author

@phadej Thanks a lot for your response! Your workarounds really help me 👍

phadej added a commit to phadej/cabal that referenced this issue Jan 29, 2018
phadej added a commit to phadej/cabal that referenced this issue Jan 29, 2018
This is not a proper fix, but we are more conservative than is strictly
required. I.e. each "branch" must have `type`.

Also as `type` is set, then `main-is` (or module) have to be specified,
because of `validate*` functions.

The `onAllBranches` is wrong. But we can fix (i.e. relax) it for Cabal-2.4
phadej added a commit to phadej/cabal that referenced this issue Jan 30, 2018
This is not a proper fix, but we are more conservative than is strictly
required. I.e. each "branch" must have `type`.

Also as `type` is set, then `main-is` (or module) have to be specified,
because of `validate*` functions.

The `onAllBranches` is wrong. But we can fix (i.e. relax) it for Cabal-2.4

/Note:/
regressions/issue-5055.format: it's broken as there isn't `main-is`
`os(windows) conditional branch in the formatted output.
23Skidoo added a commit that referenced this issue Jan 31, 2018
Make Cabal-2.2 behave like Cabal-2.0 related to #5055
@23Skidoo
Copy link
Member

Fixed by #5076.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants