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

cabal check flag names #4687

Merged
merged 6 commits into from
Aug 14, 2017
Merged

cabal check flag names #4687

merged 6 commits into from
Aug 14, 2017

Conversation

phadej
Copy link
Collaborator

@phadej phadej commented Aug 14, 2017

Related to #4686


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.

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

@phadej
Copy link
Collaborator Author

phadej commented Aug 14, 2017

Do we have cabal check tests in cabal test-suite? @ezyang ?

@phadej phadej requested a review from hvr August 14, 2017 12:51
-- starts with dash
invalidFlagName ('-':_) = True
-- mon ascii letter
invalidFlagName cs = any (\c -> not (isAscii c) && isAlpha c) cs
Copy link
Member

Choose a reason for hiding this comment

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

why not warn about any non-ascii flags?

invalidFlagName cs = any (not . isAscii) cs

@phadej
Copy link
Collaborator Author

phadej commented Aug 14, 2017

Currently

[FL973] ~/mess/lead-dash % cat lead-dash.cabal 
name:                lead-dash
version:             0.1.0.0
synopsis:            Lorem
description:         Lorem Ipsum
license:             BSD3
license-file:        LICENSE
author:              Oleg Grenrus
maintainer:          oleg.grenrus@iki.fi
category:            Testing
build-type:          Simple
extra-source-files:  ChangeLog.md
cabal-version:       >=1.10

flag -foobar
  manual: True
  default: False

library
  exposed-modules:     LeadDash
  build-depends:       base >=4.9 && <4.10
  hs-source-dirs:      src
  default-language:    Haskell2010
[FL973] ~/mess/lead-dash % cabal check                                                                                                                          
No errors or warnings could be found in the package.
[FL973] ~/mess/lead-dash % /home/ogre/Documents/other-haskell/cabal/dist-newstyle/build/x86_64-linux/ghc-8.0.2/cabal-install-2.1.0.0/build/cabal/cabal check     
The following errors will cause portability problems on other environments:
* Suspicious flag names: -foobar. To avoid ambiguity in command line
interfaces, flag shouldn't start with a dash. Also for better compatibility,
flag names shouldn't contain non-ascii characters.

Hackage would reject this package.
[FL973] ~/mess/lead-dash % cabal configure                                                                                                                      
Resolving dependencies...
Configuring lead-dash-0.1.0.0...
[FL973] ~/mess/lead-dash % /home/ogre/Documents/other-haskell/cabal/dist-newstyle/build/x86_64-linux/ghc-8.0.2/cabal-install-2.1.0.0/build/cabal/cabal configure
Resolving dependencies...
Invalid flag assignment: --foobar
CallStack (from HasCallStack):
  error, called at ./Distribution/ReadE.hs:46:24 in Cabal-2.1.0.0-inplace:Distribution.ReadE

I.e. -foobar is still allowed in GenericPackageDescription parser, but you cannot really use it.

ATM it's simpler to disallow leading dash, as you cannot use it with current --constraint syntax, alternatively one could make --constraint +-flag work. I'm not sure it's worth the trouble.

Let's see what Travis says.

@23Skidoo
Copy link
Member

ATM it's simpler to disallow leading dash

+1 on that.

@phadej phadej mentioned this pull request Aug 14, 2017
3 tasks
@@ -2162,7 +2162,8 @@ Configuration Flags

Flag section declares a flag which can be used in `conditional blocks`_.

A flag section may contain the following fields:
Flag names are case-insensitive and must match ``[a-z0-9_][a-z0-9_-]*``
Copy link
Member

@hvr hvr Aug 14, 2017

Choose a reason for hiding this comment

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

That limitation only applies to Hackage as a point of distribution, but outside of Hackage developers are free to name their packages, module names and flags and descriptions in a more i18n way.

Here's a perfectly valid .cabal file including unicode package names and flag names:

name:                無
version:             0
synopsis:            The canonical non-package
build-type:          Simple
cabal-version:       >=1.10

source-repository head
  Type:     git
  Location: https://github.com/hvr/-.git

flag 無
  description: 無

library
  default-language:    Haskell2010

  exposed-modules: Ω

  if !flag(無)
    buildable:False

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'll change it to [[:alnum:]_][[:alnum:]_-]* and add a note about Hackage format.

@phadej phadej merged commit a78ce90 into haskell:master Aug 14, 2017
@phadej phadej deleted the cabal-check-flags branch August 14, 2017 23:59
@hvr hvr mentioned this pull request Aug 15, 2017
3 tasks
tfausak added a commit to tfausak/cassava that referenced this pull request Sep 8, 2017
This flag name was changed without comment in 6e1ff78. It has caused a few problems:

- commercialhaskell/stack#3345
- commercialhaskell/stackage#2755
- commercialhaskell/stackage#2759
- commercialhaskell/stackage#2842
- haskell/cabal#4686
- haskell-hvr#150

Those problems either caused or accelerated some (attempted) changes in Cabal:

- haskell/cabal#4654
- haskell/cabal#4687
- haskell/cabal#4696

Those problems also caused a change in Stack:

- commercialhaskell/stack#3349

In short: Cabal never had any trouble with this. Stack did. The current master version of Stack can handle flags like this, but no released versions of it can. It's not clear when the next version of Stack will be released. In the meantime, no Stack users can use this package. This commit changes the offending flag to something that Stack can handle. By using a flag that Stack can handle, Stack users can once again use this package, and it can return to Stackage. There are no apparent downsides to using a more compatible flag name.
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.

None yet

3 participants