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

[#2005] Fix Formatting When Brittany Returns Warnings #2036

Merged
merged 2 commits into from Aug 6, 2021

Conversation

prikhi
Copy link
Contributor

@prikhi prikhi commented Jul 23, 2021

This is the same function I have written for my brittany PR(lspitzner/brittany#351) but there hasn't been any movement on that yet.

It'd be great if we could have this temporary fix merged until something similar is supported by upstream brittany.

@prikhi prikhi force-pushed the fix-brittany-formatting-on-warnings branch from 0c1a0e2 to cf3f3e4 Compare July 23, 2021 18:39
@jneira
Copy link
Member

jneira commented Jul 24, 2021

Thanks for fixing this here too. We have to remember remove it when brittan includes it.
@Ailrun @pepeiborra and others are you fine with include this here?

@pepeiborra
Copy link
Collaborator

No objections from me as long as licensing issues are taken into consideration, given that Brittany is LGPL and the plugin is Apache 2.0

@prikhi
Copy link
Contributor Author

prikhi commented Jul 24, 2021

We have to remember remove it when brittany includes it.

I'm fine w/ being the one responsible for removing any relevant changes if/once my brittany PR gets merged + released.

Brittany is LGPL and the plugin is Apache 2.0

Sorry, hadn't considered this aspect of it. Don't have much knowledge of these two licenses, but it seems like the only way to bring this change into HLS before brittany merges it would be for us to use my brittany fork instead of pulling the code directly into this repo?

I could attempt to write a new function that's not just a re-write of parsePrintModule w/ different return types. Have I spent too much time looking at the brittany implementations that anything I write could be considered a "derivative work"? 🤷 I don't have enough exposure to these sort of licensing issues to know what to do in this situation...

@pepeiborra
Copy link
Collaborator

pepeiborra commented Jul 25, 2021

We have to remember remove it when brittany includes it.

I'm fine w/ being the one responsible for removing any relevant changes if/once my brittany PR gets merged + released.

Brittany is LGPL and the plugin is Apache 2.0

Sorry, hadn't considered this aspect of it. Don't have much knowledge of these two licenses, but it seems like the only way to bring this change into HLS before brittany merges it would be for us to use my brittany fork instead of pulling the code directly into this repo?

I could attempt to write a new function that's not just a re-write of parsePrintModule w/ different return types. Have I spent too much time looking at the brittany implementations that anything I write could be considered a "derivative work"? 🤷 I don't have enough exposure to these sort of licensing issues to know what to do in this situation...

In my previous comment I mistakenly said brittany is LGPL, when I actually meant AGPL. This is what the AGPL license says about modified source code:

  1. Conveying Modified Source Versions.

You may convey a work based on the Program, or the modifications to
produce it from the Program, in the form of source code under the
terms of section 4, provided that you also meet all of these conditions:

a) The work must carry prominent notices stating that you modified
it, and giving a relevant date.

b) The work must carry prominent notices stating that it is
released under this License and any conditions added under section
7. This requirement modifies the requirement in section 4 to
"keep intact all notices".

c) You must license the entire work, as a whole, under this
License to anyone who comes into possession of a copy. This
License will therefore apply, along with any applicable section 7
additional terms, to the whole of the work, and all its parts,
regardless of how they are packaged. This License gives no
permission to license the work in any other way, but it does not
invalidate such permission if you have separately received it.

While you could plausibly write a new version of the function from scratch, I would not encourage this.

So in order to go ahead with this PR, you would need to change the plugin license to be AGPL. This is fine by me since I have stopped using Brittany in all my projects due to the odd choice of license, but you would also need to obtain permission from all other contributors to this plugin (@Ailrun @berberman @jneira @wz1000 and any others who have made any changes).

@jneira
Copy link
Member

jneira commented Jul 25, 2021

Well include Brittany itself introduces LGPL in hls so I dont see any problem in change the plugin license as well.
So if nobody disagree I would let the pr change it

@prikhi prikhi force-pushed the fix-brittany-formatting-on-warnings branch from cf3f3e4 to 7d3d960 Compare July 31, 2021 03:23
@prikhi
Copy link
Contributor Author

prikhi commented Jul 31, 2021

I rebased & switched the cabal license field & LICENSE file to AGPL-3.0-only. brittany uses an invalid SPDX identifier AGPL-3, which I assumed meant v3.0 only - but could switch to AGPL-3.0-or-later if deemed acceptable.

@prikhi
Copy link
Contributor Author

prikhi commented Jul 31, 2021

Also, is it possible to switch back to Apache once the upstream PR has been merged and we can remove this function? I left a comment in the cabal file suggesting so, but can remove it if it's not accurate.

@jneira
Copy link
Member

jneira commented Jul 31, 2021

Also, is it possible to switch back to Apache once the upstream PR has been merged and we can remove this function? I left a comment in the cabal file suggesting so, but can remove it if it's not accurate.

i think it s not necessary, we cant include the plugin without brittany itself so if we have to exclude gpl software, we have to exclude both at same time. In fact i think is better to keep in sync both licenses to make it clear.

So i will merge this if no contributor manifest their opposition

@prikhi prikhi force-pushed the fix-brittany-formatting-on-warnings branch from 7d3d960 to 1f95e0b Compare July 31, 2021 18:43
@prikhi
Copy link
Contributor Author

prikhi commented Jul 31, 2021

i think it s not necessary

👍, removed the comment.

@jneira
Copy link
Member

jneira commented Aug 2, 2021

reruning tests, it failed with a transient error (i hope)

@jneira jneira added the merge me Label to trigger pull request merge label Aug 2, 2021
Add a temporary fix for issue haskell#2005 while we wait for upstream brittany
to incorporate similar changes.
@prikhi prikhi force-pushed the fix-brittany-formatting-on-warnings branch from 2151dd2 to c17d43c Compare August 5, 2021 18:39
Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for fixing here the bug, please, remember to remove it once it is upstreamed

@mergify mergify bot merged commit dab8d00 into haskell:master Aug 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants