Skip to content

Make Cabal-2.2 behave like Cabal-2.0 related to #5055 #5076

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

Merged
merged 1 commit into from
Jan 31, 2018

Conversation

phadej
Copy link
Collaborator

@phadej phadej commented Jan 29, 2018

Please include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.
  • If the change is docs-only, [ci skip] is used to avoid triggering the build bots.

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

@phadej
Copy link
Collaborator Author

phadej commented Jan 29, 2018

@23Skidoo ready for the review. This is quick'n'dirty backport of Cabal-2.0 functionality.

@phadej phadej changed the title Add (failing) tests for issue #5055 [ci skip] Make Cabal-2.2 behave like Cabal-2.0 related to #5055 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

/Note:/
regressions/issue-5055.format: it's broken as there isn't `main-is`
`os(windows) conditional branch in the formatted output.
Copy link
Member

@23Skidoo 23Skidoo left a comment

Choose a reason for hiding this comment

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

LGTM.

build-depends:
base >=4.8 && <5

if os(windows)
Copy link
Member

Choose a reason for hiding this comment

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

A pretty printer bug?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

... as this is "bug-compatible" behaviour, with cabal-install-2.0. cabal-2.0 formats the input as

name: issue
version: 5055
cabal-version: >=2.0
build-type: Simple
license: BSD3
synopsis: no type in all branches
description:
    no type in all branches.
category: Test

executable  flag-test-exe
    main-is: FirstMain.hs

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

There is no conditional section at all

Copy link
Member

Choose a reason for hiding this comment

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

OK, fine, let's merge as is.

@@ -306,6 +307,16 @@ goSections specVer = traverse_ process
commonStanzas <- use stateCommonStanzas
name' <- parseUnqualComponentName pos args
flib <- lift $ parseCondTree' (foreignLibFieldGrammar name') commonStanzas fields

let hasType ts = foreignLibType ts /= foreignLibType mempty
Copy link
Member

Choose a reason for hiding this comment

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

foreignLibType mempty

Can we write mempty :: ForeignLibType or ForeignLibTypeUnknown instead? Likewise for test suites and benchmarks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd like to refactor that soon after the branch is cut...

Copy link
Member

Choose a reason for hiding this comment

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

Fine.

-- Branches
-------------------------------------------------------------------------------

-- Check that a property holds on all branches of a condition tree
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth mentioning that this function is copy-pasted from D.PD.Parse.

@23Skidoo 23Skidoo merged commit 6750de6 into haskell:master Jan 31, 2018
@23Skidoo
Copy link
Member

Merged, thanks!

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

Successfully merging this pull request may close these issues.

2 participants