Skip to content

Issue 5086 #5087

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Issue 5086 #5087

wants to merge 3 commits into from

Conversation

phadej
Copy link
Collaborator

@phadej phadej commented Jan 31, 2018 •
edited
Loading

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 added a commit to phadej/cabal that referenced this pull request Jan 31, 2018
@phadej
Remove 'ExecutableScopeUnknown'.
442dcf8 
Solves haskell#5087
Related to haskell#5003

Note: `buildable: ` field has `All` semantics. `scope: ` has
`AnyPrivate` semantics. Both might be surprising, but are "logical".

https://hackage.haskell.org/package/Cabal-2.0.1.1/docs/src/Distribution.PackageDescription.Parse.html#line-248
@phadej phadej requested a review from 23Skidoo February 1, 2018 08:30
23Skidoo
23Skidoo approved these changes Feb 1, 2018
View reviewed changes
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 modulo some comments.

Cabal/Distribution/PackageDescription/Check.hs Outdated
checkLib l = mn `elem` exposedModules l && checkExts (l ^. L.defaultExtensions)

checkBI :: BuildInfo -> Bool
checkBI bi = mn `elem` otherModules bi && checkExts (bi ^. L.defaultExtensions)
Copy link
Member

Choose a reason for hiding this comment

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

Should we also check autogen-modules?

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 guess we can

Cabal/Distribution/Types/PackageDescription.hs Outdated
@@ -369,22 +369,15 @@ withForeignLib pkg_descr f =
-- dependencies.
allBuildInfo :: PackageDescription -> [BuildInfo]
allBuildInfo pkg_descr = [ bi | lib <- allLibraries pkg_descr
Copy link
Member

Choose a reason for hiding this comment

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

There should be a mention in the changelog that the semantics of this function changed. Also the Haddock comment should be updated.

@23Skidoo
Copy link
Member

23Skidoo commented Feb 1, 2018

/cc @mgsloan

@23Skidoo
Copy link
Member

23Skidoo commented Feb 1, 2018

Looks like the test needs to be fixed.

@phadej
Copy link
Collaborator Author

phadej commented Feb 1, 2018

I have to use cabal-version: 2.2. Good. Will fix ASAP

@23Skidoo
Copy link
Member

23Skidoo commented Feb 1, 2018

There's still a test failure on AppVeyor.

23Skidoo
23Skidoo reviewed Feb 1, 2018
View reviewed changes
Cabal/Distribution/Types/PackageDescription.hs
-- all buildable executables, test suites and benchmarks. Useful for gathering
-- dependencies.
-- | All 'BuildInfo' in the 'PackageDescription':
-- libraries, executables, test-suites and benchmarks.
Copy link
Member

Choose a reason for hiding this comment

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

You forgot foreign libraries :-).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

libraries

@phadej
Copy link
Collaborator Author

phadej commented Feb 1, 2018

I hardcoded cabalVersion to be 2.2 (part of #5094). I'll make a sweep to remove 2.1 and 1.25 occurrences from the code.

@23Skidoo
Copy link
Member

23Skidoo commented Feb 1, 2018

Still seems to fail, plus there's now a merge conflict. I'm not force-pushing to your branch because the PR gets closed when a maintainer does that.

@phadej phadej force-pushed the issue-5086 branch 2 times, most recently from 51c8292 to 5cd449d Compare February 2, 2018 12:00
@phadej
Copy link
Collaborator Author

phadej commented Feb 2, 2018 •
edited
Loading

the problem with CustomPlain test is that it fails locally for me but for different reason:

ghc: can't find a package database at /home/ogre/cabal/cabal-testsuite/PackageTests/CustomPlain/cabal.dist/work/./dist/packagedb/ghc-8.2.2

EDIT and now as I fixed that, I cannot get it to fail locally.

phadej added 3 commits February 2, 2018 14:46
@phadej
Add tests Paths_ autogen module + default.extensions
fe66777 
Fixes haskell#5086

The haskell#5054 links to
commercialhaskell/stack#3789 which says

- `Ensure you have OverloadedStrings and RebindableSyntax extensions
  enabled.`

So we warn only in that case. Only `OverloadeStrings` (or
`OverloadedLists`) or `RebindableSyntax` seems to be ok.

Also make `allBuildInfos` return all (not only buildable) build infos,
removing FIXME. `allBuildInfos` is used only in D.PD.Check.
@phadej
OverloadedStrings or OverloadedLists are ok without RebindableSyntax
33e6540
@phadej
Hard-code cabalVersion. Currently 2.2
1f8e8b2
@phadej phadej mentioned this pull request Feb 2, 2018
Merged
@23Skidoo
Copy link
Member

23Skidoo commented Feb 2, 2018

Closing in favour of #5097.

@23Skidoo 23Skidoo closed this Feb 2, 2018
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@23Skidoo 23Skidoo

Assignees
No one assigned
Labels
blocked: action-required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@phadej @23Skidoo