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

Restore Distribution.Simple.Build.initialBuildSteps to Cabal's API #9856

Closed
mpilgrem opened this issue Mar 30, 2024 · 24 comments · Fixed by #9950
Closed

Restore Distribution.Simple.Build.initialBuildSteps to Cabal's API #9856

mpilgrem opened this issue Mar 30, 2024 · 24 comments · Fixed by #9950

Comments

@mpilgrem
Copy link
Collaborator

mpilgrem commented Mar 30, 2024

#9474 removed initialBuildSteps as unused, and it is absent from Cabal-3.11.0.0 (ships with GHC 9.10.0.20240328), but the function is used by Stack in the 'shim' that it puts in place to set up Setup.hs repl uses to cause Cabal to create the autogenerated files for every configured component, without building everything else: commercialhaskell/stack#6540.

Please can the function be restored to Cabal's API. Elsewhere (Matrix), @alt-romes has indicated that should be possible, tagging @sheaf.

@hasufell
Copy link
Member

Can cabal please add something to their CI so that new releases don't break one of the main consumers of the API?

@mpilgrem
Copy link
Collaborator Author

@hasufell, I think that (Cabal CI anticipating Stack's use) would be difficult - not least because each version of stack depends on one, and only one, version of Cabal. Stack also tries to build a 'setup' executable with a 'shim' for Stack's use of ghc --interactive using the Cabal boot library that ships with the specified version of GHC.

On one perspective, the 'alpha' process for GHC and its (unreleased) boot packages has 'done its job' in that it has allowed the Stack project to identify this.

@sheaf
Copy link
Collaborator

sheaf commented Apr 3, 2024

I think it is not correct to manually call initialBuildSteps externally, because this function does not take into account any pre-building that is specific to the package (e.g. custom logic generating modules in a custom setup, or a pre-build hook generating modules with Hooks build-type), nor does it run pre-processors that Cabal knows about (such as alex, happy or hsc2hs). So you would not then, in general, be able to call ghc --interactive and expect it to work.

The only practical way to create all autogenerated modules that works across all build-types that I can think of right now is to call Setup repl (possibly with extra arguments indicating which components to pre-build, if we don't want all of them). This should not cause more building than you want, but perhaps you will want to pass --repl-no-load as well. It would be worth experimenting, and if nothing suits your use case we can try to come up with a better solution.

@gbaz
Copy link
Collaborator

gbaz commented Apr 3, 2024

I think you're probably right about its limitations. However, the ticket notes that existing versions of stack will call this via a shim with whatever version of the cabal shipped with ghc is. So we should encourage stack to not do this, for the reasons you enumerate, but we should probably put it back anyway, so as to not break all existing stacks with the new ghc.

Perhaps @mpilgrem can correct me if I'm mistaken on this? I see the linked ticket describes a workaround in new stack -- does that suffice, or would it be better to restore this function but perhaps mark it deprecated?

@mpilgrem
Copy link
Collaborator Author

mpilgrem commented Apr 3, 2024

Thanks @sheaf and @gbaz. I too divide the Stack problem into two parts:

(1) Stack continuing to do what it has done in the past; and

(2) taking on board @sheaf's analysis here and elsewhere, Stack considering doing something different (to accommodate things other than Cabal's own autogenerated files).

That second part will take me a little time to understand why, currently, Stack does not make use of Cabal's repl but goes directly to ghc --interactive.

If the Cabal project would be willing to do so, I would prefer for the initialBuildSteps function to be restored and marked as deprecated.

However, I acknowledge that is only to avoid some CPP that currently looks like this:

-- | Temporary, can be removed if initialBuildSteps restored to Cabal's API.
-- Based on the functions of the same name provided by Cabal-3.10.3.0.
#if MIN_VERSION_Cabal(3,11,0)
-- | Runs 'componentInitialBuildSteps' on every configured component.
initialBuildSteps ::
     FilePath -- ^"dist" prefix
  -> PackageDescription  -- ^mostly information from the .cabal file
  -> LocalBuildInfo -- ^Configuration information
  -> Verbosity -- ^The verbosity to use
  -> IO ()
initialBuildSteps distPref pkg_descr lbi verbosity =
  withAllComponentsInBuildOrder pkg_descr lbi $ \_comp clbi ->
    componentInitialBuildSteps distPref pkg_descr lbi clbi verbosity

-- | Creates the autogenerated files for a particular configured component.
componentInitialBuildSteps ::
     FilePath -- ^"dist" prefix
  -> PackageDescription  -- ^mostly information from the .cabal file
  -> LocalBuildInfo -- ^Configuration information
  -> ComponentLocalBuildInfo
  -> Verbosity -- ^The verbosity to use
  -> IO ()
componentInitialBuildSteps _distPref pkg_descr lbi clbi verbosity = do
  createDirectoryIfMissingVerbose verbosity True (componentBuildDir lbi clbi)
  -- Cabal-3.10.3.0 used writeAutogenFiles, that generated and wrote out the
  -- Paths_<pkg>.hs, PackageInfo_<pkg>.hs, and cabal_macros.h files. This
  -- appears to be the equivalent function for Cabal-3.11.0.0.
  writeBuiltinAutogenFiles verbosity pkg_descr lbi clbi
#endif

@mpilgrem
Copy link
Collaborator Author

I see that Cabal-3.12.0.0 is released on Hackage today and Distribution.Simple.Build.initialBuildSteps is omitted from its API (although I could not see that was documented in its change log). If that was the intent, I'll close this issue.

@ffaf1
Copy link
Collaborator

ffaf1 commented Apr 26, 2024

@mpilgrem you are correct this should have at least been in the release notes, I think this ticket was overlooked.

How bad is this for stack? Can you work around until we make up our minds on what to do next?

@mpilgrem
Copy link
Collaborator Author

@ffaf1, Stack can, I think, work around it - the 'cost' to Stack is only some use of CPP.

@Mikolaj
Copy link
Member

Mikolaj commented Apr 26, 2024

@mpilgrem: Apologies for not signalling earlier nor clearer that this disputed change is now getting, I'm guessing, permanent. We have limited resources and the 3.12 release is late by about half a year, so I'm afraid we had to cut corners regarding communication and even any deeper introspection. Please kindly don't close the issue until we at least fix the release notes (they are on master branch, so they can easily be fixed after the release).

Of course, all options are still open and reverting or slow deprecation is still possible, but somebody would have to step up and drive the re-evaluation and reimplementation.

Is there anything else we can do to help? Again, that awkward outcome was not intentional, but OTOH quite predictable.

@hasufell
Copy link
Member

so I'm afraid we had to cut corners regarding communication

That seems bad. And I question whether that actually frees any of your time, because you're increasing back and forth communication, delayed releases of other tools etc.

@juhp
Copy link
Collaborator

juhp commented Apr 27, 2024

If this breaks existing stack versions, I think the change should be reverted.

Is there any real cost to just do that and marking it deprecated? Then hopefully future versions of stack could avoid using the function.

@ffaf1
Copy link
Collaborator

ffaf1 commented Apr 27, 2024

I am responsible for the release of Cabal 3.12. This is definitely a mistake and I need to check precisely how this was overlooked and how I can change the process so this does not happen again.

First things first: minimise disruption to stack maintainers and users.
@mpilgrem we are all ears: if I understood correctly a version of stack only depends on a specific version of Cabal, so that should not break existing stacks. Am I correct or will stack break with new GHCs?

@Mikolaj
Copy link
Member

Mikolaj commented Apr 27, 2024

check precisely how this was overlooked

My attempt at a postmortem:

  1. The author of Small refactor of component pre-build #9474 failed to notice the refactoring also changes the API (I'm guessing the author thought this was an internal implementation detail that users can't possibly depend upon).
  2. Therefore the PR had no changelog file (for minor refactorings that's legit) and so no mention of the removal or even of the existence of the PR entered the release notes.
  3. @mpilgrem discovered the PR classification error and created this ticket.
  4. Neither he nor any other disputant marked the ticket "regression in master" or "in 3.12" (I think the change was on the 3.12 release branch already when the ticket was created), so it was overlooked when scanning for release blockers.
  5. Nobody aware of this ticket, whether the PR author or a maintainer, offered a PR fixing the omission of the change from the release notes. I'm not sure why --- perhaps some people still wanted to clarify if that was a genuine API change or an internal change, perhaps others considered to revert the change, in which case release notes fix would not be needed.
  6. The discussion died down 3 weeks ago in this illegal erroneous state of code vs docs. This is a case of lack of time leading to a lack of communication, I'm afraid. The last person responding was @mpilgrem and his substantial comment has been unacknowledged.
  7. Nobody remembered to revive the discussion when calls for release blockers were made internally (e.g., at the open fortnightly meeting). This is another case of scarcity of manpower leading to lack of external and loud enough pre-release announcements and one-by-one survey of all major Cabal stakeholders, such as stack, about whether there exist any release blockers from their POV.

To be clear, I'm personally responsible for some of the missteps on this list, e.g., I saw this ticket early and I did not mark it as "regression in 3.12" on the weak grounds of "not enough info" + "enough eyes" at that early point.

@hasufell
Copy link
Member

This is another case of scarcity of manpower

Lots of projects are scarce on manpower. I think it's also a matter of priorities, awareness and processes. It's time to consider stack a major consumer of Cabal API and act accordingly, not just as an afterthought.

Communication/coordination should be the last thing to cut when short on manpower. Cut release frequency and new features, before cutting on communication.

@mpilgrem
Copy link
Collaborator Author

@ffaf1, I think you are correct: past releases of Stack will not work with versions of GHC that have Cabal-3.12.0.0 as a boot library. However, I think that is manageable:

  • I will release now a Stack 2.15.7 that has the work around. Stack has a high degree of backwards compatibility. That is, the barriers to Stack users moving to that new version should be low.

  • For many (perhaps most) Stack users, Cabal-3.12.0.0 will not 'bite' until (a) a version of GHC using it as a boot package is released and (b) Stackage provides a LTS snapshot specifying that version of GHC. Stackage usually waits until the ".2" minor version of GHC before cutting an LTS.

@Mikolaj, no need to apologise. I now understand I could have done more to label this issue when I created it.

@Bodigrim
Copy link
Collaborator

@ffaf1, I think you are correct: past releases of Stack will not work with versions of GHC that have Cabal-3.12.0.0 as a boot library.

We can simply revise past releases with Cabal < 3.12, it happened before with stack-2.7.* releases.

ffaf1 added a commit to ffaf1/cabal that referenced this issue Apr 27, 2024
Stress an API change is enough to warrant a `changelog.d` entry.
Check haskell#9856 to see why we need it.
@ffaf1 ffaf1 mentioned this issue Apr 27, 2024
2 tasks
ffaf1 added a commit to ffaf1/cabal that referenced this issue Apr 27, 2024
Stress an API change is enough to warrant a `changelog.d` entry.
Check haskell#9856 to see why we need it.
ffaf1 added a commit to ffaf1/cabal that referenced this issue Apr 27, 2024
Stress an API change is enough to warrant a `changelog.d` entry.
Check haskell#9856 to see why we need it.
ffaf1 added a commit to ffaf1/cabal that referenced this issue Apr 27, 2024
Stress an API change is enough to warrant a `changelog.d` entry.
Check haskell#9856 to see why we need it.
ffaf1 added a commit to ffaf1/cabal that referenced this issue Apr 27, 2024
Stress an API change is enough to warrant a `changelog.d` entry.
Check haskell#9856 to see why we need it.
@mpilgrem
Copy link
Collaborator Author

By way of update, a first release candidate of Stack 2.15.7 is now released.

ffaf1 added a commit to ffaf1/cabal that referenced this issue Apr 28, 2024
Stress an API change is enough to warrant a `changelog.d` entry.
Check haskell#9856 to see why we need it.
sheaf added a commit that referenced this issue Apr 29, 2024
This patch reinstates the 'initialBuildSteps' function, as it is
used by stack in its implementation of the multi-repl feature.

A warning has been added to that function: calling it does not suffice
to prepare the sources for a package, as there are other steps that one
might also need to perform:

  - running pre-processors (such as alex/happy)
  - running pre-build hooks or custom logic (in build-type: Hooks
    or build-type: Custom or Configure)

Fixes #9856
sheaf added a commit that referenced this issue Apr 29, 2024
This patch reinstates the 'initialBuildSteps' function, as it is
used by stack in its implementation of the multi-repl feature.

A warning has been added to that function: calling it does not suffice
to prepare the sources for a package, as there are other steps that one
might also need to perform:

  - running pre-processors (such as alex/happy)
  - running pre-build hooks or custom logic (in build-type: Hooks
    or build-type: Custom or Configure)

Fixes #9856
@sheaf
Copy link
Collaborator

sheaf commented Apr 29, 2024

I have put up #9950 to re-instate initialBuildSteps, and we intend to include this in Cabal 3.12.1.0. As I mentioned in other threads, I don't think this function can be relied on to prepare the sources for a package, so we need to find a better solution in the future for stack. I do agree we shouldn't outright break downstream consumers of the Cabal API, and I apologise for the oversight in removing the function in the first place.

sheaf added a commit that referenced this issue Apr 29, 2024
This patch reinstates the 'initialBuildSteps' function, as it is
used by stack in its implementation of the multi-repl feature.

A warning has been added to that function: calling it does not suffice
to prepare the sources for a package, as there are other steps that one
might also need to perform:

  - running pre-processors (such as alex/happy)
  - running pre-build hooks or custom logic (in build-type: Hooks
    or build-type: Custom or Configure)

Consumers wanting to prepare the sources of a package, e.g. in order to
launch a REPL session, are advised to run
@setup repl --repl-multi-file=<fn>@ instead.

Fixes #9856
sheaf added a commit that referenced this issue Apr 29, 2024
This patch reinstates the 'initialBuildSteps' function, as it is
used by stack in its implementation of the multi-repl feature.

A warning has been added to that function: calling it does not suffice
to prepare the sources for a package, as there are other steps that one
might also need to perform:

  - running pre-processors (such as alex/happy)
  - running pre-build hooks or custom logic (in build-type: Hooks
    or build-type: Custom or Configure)

Consumers wanting to prepare the sources of a package, e.g. in order to
launch a REPL session, are advised to run
`Setup repl --repl-multi-file=<fn>` instead.

Fixes #9856
sheaf added a commit that referenced this issue Apr 29, 2024
This patch reinstates the 'initialBuildSteps' function, as it is
used by stack in its implementation of the multi-repl feature.

A deprecation warning has been added to that function: calling it does
not suffice to prepare the sources for a package, as there are other
steps that one might also need to perform:

  - running pre-processors (such as alex/happy)
  - running pre-build hooks or custom logic (in build-type: Hooks
    or build-type: Custom or Configure)

Consumers wanting to prepare the sources of a package, e.g. in order to
launch a REPL session, are advised to run
`Setup repl --repl-multi-file=<fn>` instead.

Fixes #9856
sheaf added a commit that referenced this issue Apr 29, 2024
This patch reinstates the 'initialBuildSteps' function, as it is
used by stack in its implementation of the multi-repl feature.

A deprecation warning has been added to that function: calling it does
not suffice to prepare the sources for a package, as there are other
steps that one might also need to perform:

  - running pre-processors (such as alex/happy)
  - running pre-build hooks or custom logic (in build-type: Hooks
    or build-type: Custom or Configure)

Consumers wanting to prepare the sources of a package, e.g. in order to
launch a REPL session, are advised to run
`Setup repl --repl-multi-file=<fn>` instead.

Fixes #9856
@Mikolaj
Copy link
Member

Mikolaj commented Apr 29, 2024

While we finalize the proper gentle deprecation of the abruptly cut out feature (@sheaf: thank you very much for stepping up; anybody else: we are keen to hear your opinion), the changelogs/release notes have been fixed: https://hackage.haskell.org/package/Cabal-3.12.0.0/changelog

@sheaf
Copy link
Collaborator

sheaf commented Apr 29, 2024

Edit: Replying to deleted comment by juhp:

If this breaks existing stack versions, I think the change should be reverted.

Is there any real cost to just do that and marking it deprecated? Then hopefully future versions of stack could avoid using the function.

That's what I'm doing in #9950.

@juhp
Copy link
Collaborator

juhp commented Apr 29, 2024

@ffaf1, I think you are correct: past releases of Stack will not work with versions of GHC that have Cabal-3.12.0.0 as a boot library.

We can simply revise past releases with Cabal < 3.12, it happened before with stack-2.7.* releases.

Isn't this a runtime issue, that's how I understood it.

@mpilgrem it will hit Stackage Nightly rather sooner, so I feel you are being optimistic.

@Mikolaj
Copy link
Member

Mikolaj commented Apr 29, 2024

@juhp: I'm having trouble understanding from your comments if you are bringing up new issues we've missed or confirming we are addressing your past remarks properly. Would it be too much for me to ask you do go through your past two comments and either edit them or clarify who they are addressed to and what's the point you'd like to underscore? Thank you.

@juhp
Copy link
Collaborator

juhp commented Apr 29, 2024

Sorry I was commenting on my phone - With the revert I think all should be well.
So I deleted my first obsolete comment.

Thank you @sheaf!

@mpilgrem
Copy link
Collaborator Author

@juhp, noted and agreed on Stackage Nightly coming sooner than LTS but I still think it is manageable, because:

  • Stack 2.15.7 RC1 is released. If my 'fix' works and has not inadvertently broken something else, it will not take long to release Stack 2.15.7 and that will be available to Stackage before GHC 9.10 is;

  • Stack 2.15.5 and earlier expressly warn users (if they have not chosen to mute such warnings):

    ❯ stack build
    
    Warning: Stack has not been tested with GHC versions 9.10 and above, and using
             9.10.0.20240426, this may fail.
    
    Warning: Stack has not been tested with Cabal versions 3.12 and above, but version
             3.11.0.0 was found, this may fail.
    

    So, there is some 'expectation management' in the application itself.

sheaf added a commit that referenced this issue Apr 30, 2024
This patch reinstates the 'initialBuildSteps' function, as it is
used by stack in its implementation of the multi-repl feature.

A deprecation warning has been added to that function: calling it does
not suffice to prepare the sources for a package, as there are other
steps that one might also need to perform:

  - running pre-processors (such as alex/happy)
  - running pre-build hooks or custom logic (in build-type: Hooks
    or build-type: Custom or Configure)

Consumers wanting to prepare the sources of a package, e.g. in order to
launch a REPL session, are advised to run
`Setup repl --repl-multi-file=<fn>` instead.

Fixes #9856
Mikolaj pushed a commit that referenced this issue Apr 30, 2024
This patch reinstates the 'initialBuildSteps' function, as it is
used by stack in its implementation of the multi-repl feature.

A deprecation warning has been added to that function: calling it does
not suffice to prepare the sources for a package, as there are other
steps that one might also need to perform:

  - running pre-processors (such as alex/happy)
  - running pre-build hooks or custom logic (in build-type: Hooks
    or build-type: Custom or Configure)

Consumers wanting to prepare the sources of a package, e.g. in order to
launch a REPL session, are advised to run
`Setup repl --repl-multi-file=<fn>` instead.

Fixes #9856
@mergify mergify bot closed this as completed in #9950 Apr 30, 2024
mergify bot pushed a commit that referenced this issue Apr 30, 2024
This patch reinstates the 'initialBuildSteps' function, as it is
used by stack in its implementation of the multi-repl feature.

A deprecation warning has been added to that function: calling it does
not suffice to prepare the sources for a package, as there are other
steps that one might also need to perform:

  - running pre-processors (such as alex/happy)
  - running pre-build hooks or custom logic (in build-type: Hooks
    or build-type: Custom or Configure)

Consumers wanting to prepare the sources of a package, e.g. in order to
launch a REPL session, are advised to run
`Setup repl --repl-multi-file=<fn>` instead.

Fixes #9856

(cherry picked from commit dd74e92)

# Conflicts:
#	Cabal/src/Distribution/Simple/Build.hs
sheaf added a commit that referenced this issue Apr 30, 2024
This patch reinstates the 'initialBuildSteps' function, as it is
used by stack in its implementation of the multi-repl feature.

A deprecation warning has been added to that function: calling it does
not suffice to prepare the sources for a package, as there are other
steps that one might also need to perform:

  - running pre-processors (such as alex/happy)
  - running pre-build hooks or custom logic (in build-type: Hooks
    or build-type: Custom or Configure)

Consumers wanting to prepare the sources of a package, e.g. in order to
launch a REPL session, are advised to run
`Setup repl --repl-multi-file=<fn>` instead.

Fixes #9856

(cherry picked from commit dd74e92)

# Conflicts:
#	Cabal/src/Distribution/Simple/Build.hs
sheaf added a commit that referenced this issue Apr 30, 2024
This patch reinstates the 'initialBuildSteps' function, as it is
used by stack in its implementation of the multi-repl feature.

A deprecation warning has been added to that function: calling it does
not suffice to prepare the sources for a package, as there are other
steps that one might also need to perform:

  - running pre-processors (such as alex/happy)
  - running pre-build hooks or custom logic (in build-type: Hooks
    or build-type: Custom or Configure)

Consumers wanting to prepare the sources of a package, e.g. in order to
launch a REPL session, are advised to run
`Setup repl --repl-multi-file=<fn>` instead.

Fixes #9856

(cherry picked from commit dd74e92)
Mikolaj pushed a commit to ffaf1/cabal that referenced this issue May 1, 2024
Stress an API change is enough to warrant a `changelog.d` entry.
Check haskell#9856 to see why we need it.
Mikolaj pushed a commit that referenced this issue May 1, 2024
This patch reinstates the 'initialBuildSteps' function, as it is
used by stack in its implementation of the multi-repl feature.

A deprecation warning has been added to that function: calling it does
not suffice to prepare the sources for a package, as there are other
steps that one might also need to perform:

  - running pre-processors (such as alex/happy)
  - running pre-build hooks or custom logic (in build-type: Hooks
    or build-type: Custom or Configure)

Consumers wanting to prepare the sources of a package, e.g. in order to
launch a REPL session, are advised to run
`Setup repl --repl-multi-file=<fn>` instead.

Fixes #9856

(cherry picked from commit dd74e92)
TravisCardwell added a commit to TravisCardwell/haskell-actions-setup that referenced this issue May 20, 2024
Stack 2.15.7 is required to use GHC 9.10.1, as detailed in the following
issues:

* commercialhaskell/stack#6540
* haskell/cabal#9856
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants