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

SetupWrapper: Filtering for Setup arguments #5261

Merged
merged 1 commit into from
Jun 8, 2018

Conversation

alexbiehl
Copy link
Member

@alexbiehl alexbiehl commented Apr 15, 2018

Addresses #5260.

Since f3cacff we pass the components to generate documentation for to Setup with new-haddock. Older Cabals do not like this as reported by Herbert in #5260. This patch installs an argument filter mechanism just like the one for flag filtering. We now never pass extra arguments to Setup haddock for older Cabals.


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!

@alexbiehl alexbiehl requested a review from hvr April 15, 2018 14:49
@alexbiehl alexbiehl changed the title SetupWrapper: Argument filtering for arguments SetupWrapper: Argument filtering for Setup arguments Apr 15, 2018
@alexbiehl alexbiehl changed the title SetupWrapper: Argument filtering for Setup arguments SetupWrapper: Filtering for Setup arguments Apr 15, 2018
args_2_3_0 = []

filterHaddockFlags :: HaddockFlags -> Version -> HaddockFlags
filterHaddockFlags flags cabalLibVersion
Copy link
Member Author

@alexbiehl alexbiehl Apr 15, 2018

Choose a reason for hiding this comment

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

@harpocrates I think #5236 needs proper version gating for --haddock-quickjump on older Cabal's too! (@hvr, right?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't this PR solve the problem for quickjump too (via filterHaddockFlags)? Assuming the quickjump PR makes it into 2.3 too, --quickjump should also only be let through on Cabal >=2.3.0, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct

@23Skidoo
Copy link
Member

23Skidoo commented Apr 19, 2018

Looks okay to me. Maybe we should think about making enabled components more explicit in the setup wrapper's type signature - using extraArgs for that is a bit stringly-typed and ambiguous.

@harpocrates
Copy link
Collaborator

Is there anything blocking this from being merged?

@phadej
Copy link
Collaborator

phadej commented May 23, 2018

@alexbiehl this works for me. I used master + grayjays patch to fix one issue, and this patch fixed haddock. see jobs in haskell-servant/servant#961

@23Skidoo
Copy link
Member

23Skidoo commented Jun 8, 2018

Rebased against master and force-pushed, let's see if it passes now.

@23Skidoo
Copy link
Member

23Skidoo commented Jun 8, 2018

Code looks good, I'll merge this once CI is green.

@23Skidoo 23Skidoo merged commit 9d230da into haskell:master Jun 8, 2018
@23Skidoo
Copy link
Member

23Skidoo commented Jun 8, 2018

Merged, thanks! Sorry for the long wait.

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.

4 participants