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

Use parsec, drop parsec flag #4654

Merged
merged 5 commits into from
Aug 15, 2017
Merged

Use parsec, drop parsec flag #4654

merged 5 commits into from
Aug 15, 2017

Conversation

phadej
Copy link
Collaborator

@phadej phadej commented Aug 1, 2017

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!

@mention-bot
Copy link

@phadej, thanks for your PR! By analyzing the history of the files in this pull request, we identified @dcoutts, @ezyang and @23Skidoo to be potential reviewers.

@23Skidoo
Copy link
Member

23Skidoo commented Aug 1, 2017

We should be able to also get rid of the PARSEC=YES Travis configuration.

@phadej
Copy link
Collaborator Author

phadej commented Aug 1, 2017

yeah, I made PR early to see how CIs break. Will try to iron the issues asap.

@phadej phadej force-pushed the parsec-only branch 7 times, most recently from 11739db to 73387dc Compare August 10, 2017 22:16
appveyor.yml Outdated
@@ -18,13 +18,14 @@ install:
build_script:
- cd Cabal
- alex Distribution\Parsec\Lexer.x
- ghc --make -threaded -i -i. Setup.hs -Wall -Werror -XRank2Types -XFlexibleContexts
- ghc --make -threaded -i -i. Setup.hs -Wall -XRank2Types -XFlexibleContexts
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need to remove -Werror?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it breaks on unused warning in generated Lexer.hs

@@ -13,6 +13,7 @@
#ifdef CABAL_PARSEC_DEBUG
{-# LANGUAGE PatternGuards #-}
#endif
{-# OPTIONS_GHC -fno-warn-unused-imports #-}
Copy link
Member

Choose a reason for hiding this comment

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

👍

Makefile Outdated
lexer : $(LEXER_HS)

$(LEXER_HS) : boot/Lexer.x
alex -o $@ $^
Copy link
Member

Choose a reason for hiding this comment

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

Please use alex --ghc to generate the parser the way cabal would.

@phadej
Copy link
Collaborator Author

phadej commented Aug 11, 2017

Seems like green build \o/, please review my "let's make travis green hacks", I'll proceed with this on Monday.

Have good weekend everyone!

module Distribution.Parsec.Lexer
(ltest, lexToken, Token(..), LToken(..)
,bol_section, in_section, in_field_layout, in_field_braces
,mkLexState) where

-- [Note: boostrapping parsec parser]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd really love it if there was a THIS FILE IS AUTOMATICALLY GENERATED warning on checked in bootstrapped copy, ideally done with the tooling. Maybe one way to do this that obviously works is, "If this file's name is FOO, then this is an automatically generated copy."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Won't do. If something, then alex should add notes for files generated with it. I'm heavily 👎 of doing something manual / only for Cabal at this point.

,bol_section, in_section, in_field_layout, in_field_braces
,mkLexState) where

-- [Note: boostrapping parsec parser]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit concerned about this. See my comment #4633 (comment)

@@ -1,5 +1,4 @@
# cabal new-build
Warning: <ROOT>/custom-setup-without-cabal.cabal: This package requires at least Cabal version 99999
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is being tracked in #4681 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes

@ezyang
Copy link
Contributor

ezyang commented Aug 13, 2017

CI changes look fine. I'm mostly concerned about checking in autogenerated source code, but if you want to go and land this immediately we can figure that out later.

@phadej
Copy link
Collaborator Author

phadej commented Aug 14, 2017

@ezyang for now it's way simpler to check in Lexer.hs, as it would complicate GHC build system too.

@phadej
Copy link
Collaborator Author

phadej commented Aug 14, 2017

This is now green.

The only issue is that parsec can't handle few files (unicode-transforms in particular: #4687), so I have to work on .cabal legacy patch: preprocess some files, so new parser can accept all of current Hackage.

@phadej phadej force-pushed the parsec-only branch 2 times, most recently from 5a3308a to b15e73e Compare August 14, 2017 21:44
phadej added 2 commits August 15, 2017 03:49
- Manually generate Lexer.hs
- Temporarily disable parser-hackage-tests on appveyor (needs
  01-index.tar)
- Add root Makefile to handle commmon dev tasks (genering Lexer.hs)
@phadej phadej merged commit 3770156 into haskell:master Aug 15, 2017
@phadej phadej deleted the parsec-only branch August 15, 2017 09:41
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants