-
Notifications
You must be signed in to change notification settings - Fork 710
Allow build-type Configure to work with Components #10966
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
base: master
Are you sure you want to change the base?
Conversation
Upon closer reflection, maybe we can use use buildinfo for other items, I'll need to try
Still we'd want to run configure only one at most, as again all invocations should be identical. EIDT: Update, this does not work. |
I'll try to use a slightly different approach, by ensuring we run configure at least once. I'm sill unsure if this is sound as outlined in #10965; however it would be a strict improvement over the status quo nonetheless. |
70d2731
to
65dfbc4
Compare
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.
Pull Request Overview
A PR to update cabal's handling of build-type: Configure to work with components by distinguishing main library and executable components from other components.
- Update changelog entries to reflect significant changes.
- Adjust test output for expected configuration messages.
- Modify setup and project planning logic to bypass running configure for non-main-library/executable components.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
changelog.d/pr-10966.md | Updated changelog to describe the new behavior for build-type: Configure. |
cabal-testsuite/PackageTests/Configure/cabal.out | Revised expected output to reflect updated configure messages. |
cabal-install/src/Distribution/Client/SetupWrapper.hs | Added isMainLibOrExeComponent flag and modified build-type override logic. |
cabal-install/src/Distribution/Client/ProjectPlanning.hs | Revised configuration planning to conditionally apply build-type. |
cabal-install/src/Distribution/Client/Configure.hs | Propagated default flag value for isMainLibOrExeComponent in configuration. |
Comments suppressed due to low confidence (1)
cabal-testsuite/PackageTests/Configure/cabal.out:7
- The expected output strings have changed (e.g., updating from '(lib:zlib)' to '(lib)' and modifying the configuration message). Please confirm that all tests have been updated accordingly to reflect these new messages.
- zlib-1.1 (lib:zlib) (first run)
65dfbc4
to
902ff15
Compare
902ff15
to
007f02d
Compare
fdea24f
to
b9ff22d
Compare
It's a good thing to work on to remove these reasons for whole package builds but I'm not sure this is the right approach. Can you explain how this approach avoids the issue which is described in #4548 ? It seems that at a glance, you will still get two concurrent runs of configure (for the library and executable components) It would be good to also add a test. My instinct is that you should instead modify the |
Currently, I don't think we can simply remove the |
I first opened #10965, because I though the naive thing would be to just ensure configure is run in separate folders. Turns out, it already is. So there is no more running concurrently in the same folder, sure it still runs n times (once per component) and this is wasteful. Even worse while you can do codegen (e.g. creation of header files in configure script, well you can do all kinds of stuff after all it's just a shell script), you can't influence anything but the main library and the executable components with the .buildinfo file. The other is that the directory layout right now would even mean you aren't really in what cabal-install considers the final build directory for components.
Yes you will. But they will be in separate directories. Hooray for cabal-install being sensbile about no global CWD state anymore.
That can be added.
While making the Custom and Hooks setups separate components is indeed a worthwhile goal, I'll leave this to @andreabedini who has already been working on that for Setup/SetupHooks based build-types.
As outlined above I thought for a while if I were to call this out or not. I've got fairly thick skin and am not much bothered by this, but:
comes across extremely hostile. This change did a lot more than simply remove the |
I think there is a misunderstanding here. I am suggesting that extending the Making To be clear as well, what is the motivation for this change? Is it self-motivated or is it because you want to support
I see, that's an important difference, I tried building To summarise my thoughts:
|
I apologise, it wasn't at all my intention to dismiss the work you put into finding, implementing and testing a surgical change that improves the situation for |
I'm not sure there is. Hooks and Custom both require a proper phase distinction, they both come with haskell scripts. The hooks documentation mentions creating a
Yes, I'm glad we can all agree on this!
This is to turn a In general I see
I'm not sure I understand that line. Yes this is motivated by a public sublibraries package with
Much appreciated! Let's make |
Allow me to add one data point. I have only recently realised that Haskell.nix build cabal packages by component, always. For any build type, haskell.nix will compile Setup.hs (or use the default one if missing) and then call I only skimmed the discussion but I agree that moving configure build-type to autoconfSetupHooks and making setup a real component1 would be both great improvements. That said I think allowing build-type configure to work by component is something we can safely do right away because 1) it goes in the same direction of the other two changes above 2) I believe any unintended breakage can be dealt with by fixing the package (or moving it to setup-hooks?). Footnotes
|
I mean that If you are using @andreabedini That is interesting to know, I didn't know As explained in the previous summary, I think it is fine to merge this patch but I think there are some trade-offs. |
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 will approve once the commit message/comments are updated to reflect the discussion and there is a testcase.
Thanks @angerman :)
This is not correct. Only |
@angerman You write:
I don't understand this comment. The configure command for Whole package:
A single component:
(Note that after |
Thanks @andreabedini, you are right. I was confused by the code in |
Sure you can run it per for components other than the main library or executables (also components), but there is no way that the |
b9ff22d
to
ccc6e59
Compare
@mpickering amended the commit message, I hope it includes all the relevant information you want, if not, please let me know which ones specifically you'd like me to add. Also: test-case added. |
8732261
to
f3c4465
Compare
When components were introduced, cabal had support for build-type: Configure, which allows to run a `configure` script prior to building the package. Upon the introduction of components, it became obvious that we can't just configure each and every component, as this easily runs into the situation where the `configure` script is run potentially concurrently for all components. Due to recent changes to cabal-install and lib:Cabal, cabal is CWD aware, and concurrently executed `configure` scripts won't step on each other anymore. However, build-type is a global option for cabal files, and is usually used to produce a <pkg>.buildinfo file. This file is then used to augment the Main Library and Executables only. This change now tracks whether or not we are building a main library or executable component, and if we do, retains only for these components the build-type: Configure from the package description. For all other components, we will fall back to the default build-type: Simple. For end users who want to depend on package configured values, they will need to set their sub libraries, testsuites and benchmark components to depend on the main library, and import any configured values from there. For lib:Cabal, which doesn't know about components when configure is invoked, we will continue to execute configure. There is no impact on lib:Cabal in this change. This can appraoch may be further improved once we have turned Setup (Setup.hs/SetupHooks.hs) into a separate Setup component on which each component depends. Notably for Make, Configure, and Simple, cabal-install does not need to compiler a Setup.hs/SetupHooks.hs and can often rely on the act-as-setup (internal setup) method.
f3c4465
to
73c3061
Compare
After @mpickering approves, we still need one more review. @philderbeast: would you be inclined to provide it? |
When components were introduced, cabal had support for build-type: Configure, which allows to run a
configure
script prior to building the package. Upon the introduction of components, it became obvious that we can't just configure each and every component, as this easily runs into the situation where theconfigure
script is run potentially concurrently for all components.However, build-type is a global option for cabal files, and is usually used to produce a .buildinfo file. This file is then used to augment the Main Library and Executable components only.
This change now tracks whether or not we are building a component to the setup invocation, and if we do, ignores a build-type: Configure built-type from the package description. For end users who want to depend on
package configured values, they will need to set their sub libraries, ... and other components to depend on the main library, and import any configured values from there.
For lib:Cabal, which doesn't know about components when configure is invoked, we will continue to execute configure, even though cabal will never use that info.
Fixes #4548
significance: significant
in the changelog file.