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

Make the AGPL flag manual in cabal #250

Merged
merged 2 commits into from
Aug 5, 2020
Merged

Conversation

fendor
Copy link
Collaborator

@fendor fendor commented Jul 29, 2020

I honestly don't know what this entails and the documentation is hard to find but I think it fixes the problem described in haskell/vscode-haskell#255

@fendor fendor requested a review from alanz July 29, 2020 07:55
@pepeiborra
Copy link
Collaborator

Cabal automatically tries all the possible combinations of non-manual flags when solving, leaving manual flags at their default value. I think we really want this flag to be non-manual, otherwise the Cabal build will fail in 8.10.1 where Brittany is not available

@jneira
Copy link
Member

jneira commented Jul 29, 2020

yeah, auto makes cabal trying enable it to get a build plan so it must be manual

@fendor
Copy link
Collaborator Author

fendor commented Jul 29, 2020

@pepeiborra Brittany wasn't included for me for ghc 8.8.3 either, which is weird, since the default is true and the build succeeds with Brittany on 8.8.3. Do you know why?

Also, if Brittany does not exist for 8.10.1 yet, we should, as suggested in haskell/vscode-haskell#255, use a different default formatter, in my opinion.

@pepeiborra
Copy link
Collaborator

yeah, auto makes cabal trying enable it to get a build plan so it must be manual

Really? I thought that cabal would use the default value first, and then if solving fails, try again with the negated default value if the flag is non manual.

@ndmitchell
Copy link
Collaborator

I have a lint check for my projects that all flags are set to Manual. I think of it was manual = for user. Otherwise = random hacks to encourage Cabal to find a build plan.

@pepeiborra
Copy link
Collaborator

@pepeiborra Brittany wasn't included for me for ghc 8.8.3 either, which is weird, since the default is true and the build succeeds with Brittany on 8.8.3. Do you know why?

Also, if Brittany does not exist for 8.10.1 yet, we should, as suggested in haskell/vscode-haskell#255, use a different default formatter, in my opinion.

I have no idea why, but you can ask cabal. I agree that Brittany is not a good default in 8.10.1 !!

@fendor fendor force-pushed the cabal-flag-agpl branch 2 times, most recently from 80775bf to 7120a77 Compare July 29, 2020 09:42
@jneira
Copy link
Member

jneira commented Jul 29, 2020

@pepeiborra the mysteries of the cabal plan algorithm are unfathomable 😝
Sometimes it choose a path with the flag enabled although there could be another one without cause the latter needs more steps.
In any case I would not let a tool activate agpl automatically (with little info to the user) cause it could suppose a serious problem in some contexts, gplphobics.
If we need to disable Brittany for other causes (mainly non buildable deps) we should use another dedicated flag, imo.
We were lazy and reused the agpl flag with a totally different sense.

@Ailrun
Copy link
Member

Ailrun commented Jul 29, 2020

Also, if Brittany does not exist for 8.10.1 yet, we should, as suggested in haskell/vscode-haskell#255, use a different default formatter, in my opinion.

I thought our default formatter is ormolu, as written here

, formattingProvider = "ormolu"

Am I missing something?

@fendor
Copy link
Collaborator Author

fendor commented Jul 29, 2020

@Ailrun the vscode plugin overrides this configuration

@Ailrun
Copy link
Member

Ailrun commented Jul 29, 2020

That's... surprising 😮 Thank you for the answer :)

@lukel97
Copy link
Collaborator

lukel97 commented Jul 29, 2020

It is surprising indeed, I've opened up a PR on vscode-haskell to change the default to ormolu to match up. Unless anyone has a stronger preference for another default?

@ndmitchell
Copy link
Collaborator

I think avoiding AGPL/GPL for default things is wise - it isn't a typical Haskell license.

@fendor
Copy link
Collaborator Author

fendor commented Aug 5, 2020

@bubba Should be merge this before or after #288 ?

@lukel97
Copy link
Collaborator

lukel97 commented Aug 5, 2020

@fendor Hm not sure, depends if we think we should actually include Brittany in the binaries in the first place. #288 also builds Brittany on 8.10.1 so some of the CPP flags will need changed

@fendor
Copy link
Collaborator Author

fendor commented Aug 5, 2020

Exactly, we should coordinate. Whatever you decide in this is fine by me, but I would like to make this flag manual eventually.

@lukel97
Copy link
Collaborator

lukel97 commented Aug 5, 2020

Ok we should merge this first then, this PR makes sense to me

@fendor fendor merged commit 4cdafec into haskell:master Aug 5, 2020
pepeiborra pushed a commit that referenced this pull request Dec 27, 2020
* Fix #248 and #250

This fixes hover for types, classes and type variables.

Information about spans includes a `Maybe Type` which is `Just` for data-level
expressions and `Nothing` for type-level expressions.

`AtPoint.atPoint` which is the oddly-named function responsible for constructing
hover information, runs in the `Maybe` monad, and aborted at the first sight of
a `Nothing`, thus producing no hover information for type-level spans.

In the process of fixing this, I have refactored the function to

+ separate the construction of data-level and type-level hover info

+ make the components that make up the hover info (and their construction) more
  clear

I can see plenty little improvements that could be made to the functionality of
the code (and lots that could be made to its organization), but the most
important fixes of the basic missing functionality are here.

Fix #248
Fix #250

* Revert behaviour of locationsAtPoint to match its name

The name suggests that it returns all locations, while the last commit changed
this to return at most one.

* Fix issue numbers in test titles

There was some confusion about which tests addressed issue 248 vs 249
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.

6 participants