-
Notifications
You must be signed in to change notification settings - Fork 697
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
Remove UHC #5347
Remove UHC #5347
Conversation
40b4a9f
to
d4714d0
Compare
/cc @atzedijkstra |
Makes sense for JHC. For UHC I think it is worth double checking with the UHC authors. Back in the day both sides went to some trouble to get UHC supporting Cabal and Cabal supporting UHC. |
I keep UHC alive, that is, keep it compilable and working. Although currently not further developed I’d appreciate if the Cabal support is also kept alive, that is, kept compilable (not necessarily working), so if work on UHC is ever picked up again the code is not gone.
regards
Atze
…--
Atze Dijkstra
On 30 May 2018, at 00:33, Duncan Coutts ***@***.***> wrote:
Makes sense for JHC. For UHC I think it is worth double checking with the UHC authors. Back in the day both sides went to some trouble to get UHC supporting Cabal and Cabal supporting UHC.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
OK, let's remove JHC, but not UHC. |
Cabal/Distribution/Compiler.hs
Outdated
@@ -70,7 +69,7 @@ instance NFData CompilerFlavor where rnf = genericRnf | |||
|
|||
knownCompilerFlavors :: [CompilerFlavor] | |||
knownCompilerFlavors = | |||
[GHC, GHCJS, NHC, YHC, Hugs, HBC, Helium, JHC, UHC, Eta] | |||
[GHC, GHCJS, NHC, YHC, Hugs, HBC, Helium, Eta] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that we didn't remove Hugs, YHC or NHC from here, we should leave JHC and UHC here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure it wasn't just missed accidentally?
I understand that there might theoretically be Setup.hs code out there that relies on these, but wouldn't it be to "break it at compile time" rather than runtime?
@@ -432,17 +432,17 @@ optionsFieldGrammar | |||
optionsFieldGrammar = combine | |||
<$> monoidalFieldAla "ghc-options" (alaList' NoCommaFSep Token') (extract GHC) | |||
<*> monoidalFieldAla "ghcjs-options" (alaList' NoCommaFSep Token') (extract GHCJS) | |||
<*> monoidalFieldAla "jhc-options" (alaList' NoCommaFSep Token') (extract JHC) | |||
-- NOTE: Hugs and NHC are not supported anymore, but these fields are kept |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should either remove {hugs,nhc98}-options
from here as well, or keep jhc-options
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing those fields from the grammar would result in parser errors/warnings being emitted by newer lib:Cabal; if we want to remove them, we should only make it a parser warning/failure for newer cabal-version
specs; while older specs should be silent about the occurence.
/cc @phadej
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I've "moved" this to "knownField ..." for precisely the reasons that @hvr mentions.
@@ -730,7 +730,7 @@ instance Arbitrary CompilerFlavor where | |||
--TODO: [code cleanup] export knownCompilerFlavors from D.Compiler | |||
-- it's already defined there, just need it exported. | |||
knownCompilerFlavors = | |||
[GHC, GHCJS, NHC, YHC, Hugs, HBC, Helium, JHC, UHC] | |||
[GHC, GHCJS, NHC, YHC, Hugs, HBC, Helium] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please fix the TODO here while you're at it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I intentionally did not to avoid mixing up the "remove JHC" change with an "expand the Cabal API" change.
I'm assuming that adding the knownCompilerFlavors will implicitly expand the Cabal API... is that assumption right, and do we want that?
If so, it'll happily do a folllow-up for that.
(I suppose I could add a Haddock note about the API only being for testing, or something?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo comments.
Is JHC no longer usable/under development? |
@atzedijkstra Please reconsider asking the Cabal maintainers to keep the UHC code around. I'll preface this by saying that it actually applies to a lot of Cabal features beside UHC, so I apologize if this sounds like I'm going on a rant about UHC. It's just that I've been thing a lot about this type (Hope I'm doing the quoting thing correctly.)
I can certainly understanding wanting to have others "maintain" the code, but I think this is the wrong tradeoff -- unless you or someone else who actually works on UHC can commit to actually maintaining the UHC-related code in a working state. That would at least probably involve adding Cabal+UHC integration tests of the "can it compile a hello-world cabal package?" type. Here are my reasons:
If someone were to commit to actually making sure we keep this working by e.g. adding tests, then I'd obviously have much less of a problem with keeping it, but as-is I see only downsides to keeping it and no upside. Regards, Bardur Footnotes: [1] Obviously, if UHC were hugely popular, then the there's a reasonable case to be made that the Cabal/cabal-install developers should support it without "upstream" support simply to avoid becoming irrelevant, but sadly that's not the world we live in. Personally, I'd love to have a more diverse compiler landscape for Haskell, but it's looking like a more and more remote possibility due to the speed of change in GHC and the Haskell ecosystem's usage of the new and shiny features. EDIT: Formatting. |
For me personally, keeping UHC bits compilable is not a huge burden, but other contributors's mileage may vary. |
@23Skidoo I'm not saying that the UHC bit is by itself a problem, in isolation. It's adding its own little burden to the overall maintenance cost for every maintainer and that it simply isn't worth that extra additional cost to keep it compiling. It's like one of the "straw that ends up breaking the camel's back" type situations. |
I think this is a plain straw, not one that relates to any camel. There’s
other things that eventually should be trimmed and will bring noticable
benefit to the codebase. Here, the overhead is minimal, sonce the design of
cabal is conducive to having an enumerated list with a few more compilers
listed.
|
@gbaz It's not just an enumerated list. As I've mentioned there are a few possible API simplifications that this could lead to. (EDIT: Plus, possibly dead code that could be removed. I haven't investigated further on that.) |
@23Skidoo Any comments on my review questions/comments? |
We can merge the uncontroversial JHC bits, I think I'll do it later today. |
Cool, thanks! |
d4714d0
to
285ebf8
Compare
Merged the JHC bit, rebased against |
I too don't see that see the reason to drop UHC support. Cabal is supposed to be abstracted in a way that this shouldn't matter much. I would very much hate if Cabal becomes a GHC specific tool. In my opinion keeping other implementation around is a great way to insure the API remains abstracted cleanly. Besides the maintenance burden is quite low anyway. Popularity shouldn't be a reason to remove something. By that argument the only compiler that should be supported is GHC. |
Ok, I'm done tilting at windmills, I guess... @23Skidoo Can you please remove my "commit bit"? I guess there's just too much of a philosophical difference between me and the maintainers when even simple things like YAGNI are controversial when we're talking about a "feature" that's perhaps relevant to tens of users at most. It's just to draining to periodically get my hopes up, so I'd rather just let go completely of any idea of trying to contribute major cleanups to Cabal in future. (This state of affairs saddens me, but I guess it's just time to move on. Thanks to all the maintainers/contributors for all the hard work you do, btw.) @atzedijkstra Just out of curiosity, is the statement on UU-ComputerScience/uhc#85 (comment) about not being able to build libraries at all via Cabal+UHC still true? If so, then how on earth can it possibly be relevant to keep UHC "support" in Cabal? (I'll not that the issue has been open for 2 years now. How likely do we think it's going to be fixed/implemented?) EDIT: Oh, I forgot: You might want to know that the UHC README points to non-existent URL. |
OK, here are the commits for removing the JHC/UHC compiler flavours.
There are a couple of opportunities for removing some function parameters at this point because they've become unused, but I'm not sure we can do that per compatibility requirements...? It's mostly the
defaultInstallDirs
anddefaultInstallDirs'
functions I'm thinking of.