-
Notifications
You must be signed in to change notification settings - Fork 16
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
Move GHC.RTS.Flags out of base. #289
Comments
Does anyone feel this needs a deprecation period? |
I think that because the API is kept in such a close tandem with the compiler that we should be able to get away without a deprecation period, provided that we find a new home for the module where it remains up-to-date with the compiler as current. I suggested |
Thanks for pushing this forward @doyougnu! I agree that Users are strongly discouraged from depending on I can see a few plausible migration strategies:
|
I feel like Ultimately I think it'd be nice to see
This seems like a very useful feature to have on the |
I agree with @parsonsmatt |
Adding user-facing definitions to
Moreover it suggests as a candidate for inclusion in
Just because a package queries some information about the RTS, I don't think it should be forced to depend on GHC internals. |
I agree that users should absolutely not get the module in question from If GHC HQ really thinks that |
Agreed with @tomjaguarpaw IIRC the current plans for |
Yeah, I guess for me |
Cabal upstream issue mentioned: haskell/cabal#10384 |
I too agree with @parsonsmatt and @Ericson2314 and think
This is one such proposal. It continues:
This seems to be exactly the path forward we're discussing here.
I desperately do not want a deprecation period and I believe that a deprecation period would exacerbate the ecosystem coupling. Instead I prefer a clean break with a new GHC release. We are in a fortunate position because we still have a coupled ghc and base. So we can do the whole migration at once for a single release of ghc+base. As for the impact, the same 23 packages that were previously impacted will be impacted again. |
I don't think there will be any objection to moving the original source of If, on the other hand, you get CLC approval to remove the module from |
I think the idea is that the old version in |
Note that module GHC.RTS.Flags
( RtsTime
, RTSFlags (..)
, GiveGCStats (..)
, GCFlags (..)
, ConcFlags (..)
, MiscFlags (..)
, IoManagerFlag (..)
, DebugFlags (..)
, DoCostCentres (..)
, CCFlags (..)
, DoHeapProfile (..)
, ProfFlags (..)
, DoTrace (..)
, TraceFlags (..)
, TickyFlags (..)
, ParFlags (..)
, HpcFlags (..)
, {-# DEPRECATED "import GHC.IO.SubSystem (IoSubSystem (..))" #-}
IoSubSystem (..)
, getRTSFlags
, getGCFlags
, getConcFlags
, getMiscFlags
, getDebugFlags
, getCCFlags
, getProfFlags
, getTraceFlags
, getTickyFlags
, getParFlags
, getHpcFlags
) where
import GHC.Internal.RTS.Flags
import GHC.Internal.IO.SubSystem (IoSubSystem(..)) |
I agree with this (so perhaps we've been talking at cross purposes). The interesting question is where the definitions are exposed to users. Indeed, note that the definitions are already in But then the question remains as to how users get access to the extended API, and whether |
Ah thank you, so my comments have been slightly misguided then! But I still think they should be exported from a package officially maintained by GHC HQ, blessed as the official way of obtaining them. I don't mind what that package is, but it shouldn't be |
I'd be happy to maintain (or co-maintain!) something like this. There's a few more bits of But also it probably wouldn't be too bad to just expose these from |
Let's recap the conversation:
Candidate targets:
But the flags are not intended to "have their ultimate home in base" and are not really experimental features. That is the entire point of this proposal! Therefore we can exclude
So I think a new packages similar to
This reaches the goal, acknowledges the problem, and decouples ghc from base a little bit more than yesterday. The extra cost would be maintenance for this package but that is mitigated by keeping it under GHCHQ control. The roadmap should be straight forward:
@TeofilC and everyone else. What else is in |
Great summary! I think some of this was discussed in #146 So maybe it is just |
There's also relevant discussion at https://gitlab.haskell.org/ghc/ghc/-/issues/25242: the new I'm still not seeing a compelling reason not to use
I think this would be great to see as a separate package, which:
|
@adamgundry Are you suggesting to use |
I think from a user perspective it doesn't much matter, does it? Whether the definition exists in My claim is that
|
There are some things being discussed here that don't affect The bottom line for the CLI is this:
This is close to what @doyougnu is saying, except reflecting the fact that the current implementation is already in So which choice are we doing? Once we decide on the decision for |
Hello, we discussed this in the latest GHC call. GHCHQ's view is to re-export these flags from Is there anything left to discuss or can a vote begin? |
Just to make sure, was the opinion in the call, that it would actually be better for the current flags to stay in base, (e.g., less people depending on ghc-experimental) or that it is just less hazzle to keep it? I have basically no stakes in using those flags, but I could imagine it getting confusing to users that there are a few flags which they can’t import from the place from which they get all others. In the long term only exposing them from ghc-experimental seems cleaner to me? |
The opinion was to keep the flags in base for the time being to prevent more breakage, but to eventually remove them.
I agree and this is cleaner to me as well. IMHO there should just be a single source of truth for something like this. So I do not mean to imply that the flags in Did that clear things up? |
Just one last question: What exactly do you want the CLC to vote about? If there is no change to the base API intended now, there is nothing to vote on, is there? |
Ah haha yea I guess that is right. Then I'm comfortable to park this until the changes in |
I'm not entirely sure what is being asked of me now. I believed I had provided a proposal in the OP comment and provided two plans for implementation and requested a vote in the Since that comment we have decidedNot-clean break cannot simply re-exportIf the implementation just re-exports then typeclasses will additional type class instances will leak. Even if we re-make the API, type classes in
|
FWIW, |
There seems to be a confusion, the original plan of @adamgundry does not leak any instances. |
Great. I've edited my previous comment. Can the CLC please vote on this now? |
What are we being asked to vote on? Normally CLC votes on one specific proposal, with associated MR. But perhaps in this case you're asking the CLC to choose between "clean break" and "not-clean break" (as per #289 (comment))? (Personally I support the not-clean break and oppose the clean break. |
Yes this is what I am asking. I could open another proposal for the other plan but I think that is just redundant and I don't exactly want to issue an MR just to have the plan that the MR represents be rejected. Per the proposal document, an MR is required when requested. It was not my understanding that such a request has been made. |
Yes, makes sense to me. My vote would be
|
I'm +1 on the plan in #289 (comment) and -1 on other suggestions. (To be clear: this vote is non-binding and advisory only) |
I'm +1 for both. Or let's say: I don't really care. This type of API is something that is naturally breaking all the time. So I feel doing elaborate deprecation is kinda moot. But if people want that, go ahead. |
@velveteer @parsonsmatt @angerman @mixphix any more non-binding opinions to help @doyougnu to choose the further course of action? |
I'm with @tomjaguarpaw on this one. My vote would also be
|
@doyougnu is my understanding correct that you settled to implement #289 (comment)? Would you like CLC to vote? |
Yes that is correct. Thanks for the followup! |
@doyougnu, can you update the proposal to reflect the current plan? In particular, there are several mentions of |
@bgamari done. Here are the changes I added to the implementation section:
|
Dear CLC members, let's vote on the proposal to add a compatibility layer to @tomjaguarpaw @parsonsmatt @hasufell @angerman @velveteer @mixphix +1 from me. We've been inundated with proposals to add GHC RTS flags (#288, #263, #254, #243), which isn't productive either for CLC or for GHC team, so uncoupling (We shall probably deprecate and eventually remove them from |
+1 |
2 similar comments
+1 |
+1 |
Thanks all, that's enough votes to approve. |
Thanks all for your time, thoughts and labor! |
Thank you for driving this forward! |
What
Presently, we expose
GHC.RTS.Flags
in base. This proposal proposes to move them out ofbase
and intoghc-internal
or some other similar package. We can decide where would be best in the comments.Why
It is non-obvious that have
GHC.RTS.Flags
in base creates downstream issues, but in practice this is the case and we end up creating more work for the CLC, for GHC developers, increase coupling of base to GHC versions. There are several CLC proposals which were only created because these flags are exposed in base, and as far as I can tell there isn't much debate in any of the proposals:--no-automatic-time-samples
inGHC.RTS.Flags
API #243: similar to Add new RTS options introduced by eras profiling to GHC.RTS.Flags #254Which begs the question: Is there a meaningful debate to be had for RTS flag changes like this? And should that debate occur in base with the CLC. So far these seem to have been relatively uncontroversial.
Implementation
I will implement and this should miss the 9.12 fork.
I'm unsure ifghc-internal
is the best home for these flags. I'll bring this up in the next GHC call. But I think it is the best place we have right now.The plan is given by this comment. To be succinct we:
ghc-experimental
and expose them throughghc-experimental
. This is where consumers should expect to use these flags moving forward. This is MR!13343, it still needs work but should be trivial I just won't have time until this Friday.Impact
As Duncan writes in #263:
This is exactly right, and so decoupling these flags is a meaningful step towards the ghc-base split.
The text was updated successfully, but these errors were encountered: