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

Introduce checks for upper dependencies in setup. #5370

Closed
wants to merge 3 commits into from
Closed

Conversation

qnikst
Copy link
Collaborator

@qnikst qnikst commented Jun 9, 2018

setup depends are now checked for having upper dependencies.
Upper bounds are mandatory for base and Cabal libraries, and
are optional (but emit warning) for other libraries.
Implicit dependencies are not being checked.

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!

@23Skidoo
Copy link
Member

23Skidoo commented Jun 9, 2018

Re: tests, something like PackageTests/AutogenModules/Package/setup.test.hs will do (basically just run configure or check and then use assertOutputContains to check that the messages are present).

@23Skidoo
Copy link
Member

23Skidoo commented Jun 9, 2018

Implicit dependencies are not being checked.

You mean for pre-setup-depends build-type: Custom packages?

@qnikst
Copy link
Collaborator Author

qnikst commented Jun 9, 2018

I don't cover cases like: #4683 (comment) or if dependency on a Cabal exists due to the dependency on the other package.

@23Skidoo
Copy link
Member

23Skidoo commented Jun 9, 2018

That's fine.

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 minor comments.

| otherwise = emitWarning nm
emitError nm =
PackageDistInexcusable $
"The dependency 'build-depends: '"++nm++"' does not specify an "
Copy link
Member

Choose a reason for hiding this comment

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

s/build-depends/setup-depends/.

Right (pkgs', _) ->
Map.fromListWith intersectVersionRanges
[ (pname, vr)
| Just spi <- pure $ setupBuildInfo pkgs'
Copy link
Member

Choose a reason for hiding this comment

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

I think you can use maybeToList here. Also, "spi" should be "sbi" ("bi" by convention means "build info").

setup depends are now checked for having upper dependencies.
Upper bounds are mandatory for base and Cabal libraries, and
are optional (but emit warning) for other libraries.
Implicit dependencies are not being checked.
@qnikst
Copy link
Collaborator Author

qnikst commented Jun 9, 2018

@23Skidoo I've addressed your comments and added a test case.

@quasicomputational
Copy link
Contributor

Coincidentally, I've just implemented making check play nicely with cabal-testsuite in #5372. I could split that commit off into its own PR and merge that if you think it'd make this PR's test any nicer.

@23Skidoo
Copy link
Member

A bunch of solver tests now fail...

@@ -0,0 +1,24 @@
import Test.Cabal.Prelude

-- Test that setup shows all the 'autogen-modules' warnings.
Copy link
Member

Choose a reason for hiding this comment

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

Copy-pasto.

@23Skidoo
Copy link
Member

I could split that commit off into its own PR and merge that if you think it'd make this PR's test any nicer.

Thanks, but I think we can update the tests that run cabal check after your PR has been merged.

@qnikst
Copy link
Collaborator Author

qnikst commented Jun 10, 2018

I'll need to make this change more complex, currently cabal does not distinguish between -any and no dependency set at all. As a result it's not possible to silence the warning by passing -any.

@23Skidoo
Copy link
Member

23Skidoo commented Jun 10, 2018

The change we want to do is replace data Dependency = Dependency PackageName VersionRange with data Dependency = Dependency PackageName (Maybe VersionRange) so that it'd be possible to distinguish between build-depends: foo -any and build-depends: foo.

@ip1981
Copy link

ip1981 commented Jun 8, 2020

For a mandatory upper bound (at Hackage if I recall) I set <50, how does it help?

@gbaz
Copy link
Collaborator

gbaz commented Aug 13, 2021

closing in favor of #7409

@gbaz gbaz closed this Aug 13, 2021
@fgaz fgaz deleted the gh-5317 branch August 13, 2021 16:35
mergify bot pushed a commit that referenced this pull request Aug 26, 2021
* Introduce checks for upper dependencies in setup.

setup depends are now checked for having upper dependencies.
Upper bounds are mandatory for base and Cabal libraries, and
are optional (but emit warning) for other libraries.
Implicit dependencies are not being checked.

* Add tests.

* Fix tests.

* add changelog

* fix test for the newer cabal_raw'

* scale down alerts: no warnings on any package, only errors on base+Cabal

* address review comments

* typo

* improve changelog (add Cabal)

* remove boundedAbove and use Distribution.Version.hasUpperBound instead

Co-authored-by: Alexander Vershilov <alexander.vershilov@gmail.com>
Co-authored-by: Emily Pillmore <emilypi@cohomolo.gy>
Co-authored-by: Mikolaj Konarski <mikolaj@well-typed.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants