Skip to content

Call writeAutogenFiles in configure #6441

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

Closed

Conversation

fendor
Copy link
Collaborator

@fendor fendor commented Dec 17, 2019


Please include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.
  • If the change is docs-only, [ci skip] is used to avoid triggering the build bots.

Please also shortly describe how you tested your change. Bonus points for added tests!

Aims to fix #2209. Generates Paths_*.hs and cabal_macros.h in configure phase.
We do that by moving the function writeAutogenFiles into the Configure.hs file and generating the files after the LocalBuildInfo has been created.

This PR has only been manually tested. Deleted dist-newstyle in a project.
Then made sure that cabal build --only-configure generates both Paths_*.hs and cabal_macros.h.
Roughly, the following tree was generated:

`dist-newstyle` after `cabal build --only-configure`
> tree dist-newstyle/
dist-newstyle/
├── build
│   └── x86_64-linux
│       └── ghc-8.6.4
│           └── fprog-0.1.0.0
│               ├── build
│               │   └── autogen
│               │       ├── cabal_macros.h
│               │       └── Paths_fprog.hs
│               ├── cache
│               │   └── config
│               ├── setup-config
│               └── x
│                   └── intcode
│                       ├── build
│                       │   └── intcode
│                       │       └── autogen
│                       │           ├── cabal_macros.h
│                       │           └── Paths_fprog.hs
│                       ├── cache
│                       │   └── config
│                       └── setup-config
├── cache
│   ├── compiler
│   ├── config
│   ├── elaborated-plan
│   ├── improved-plan
│   ├── plan.json
│   ├── solver-plan
│   ├── source-hashes
│   └── up-to-date
├── packagedb
│   └── ghc-8.6.4
│       ├── package.cache
│       └── package.cache.lock
└── tmp

Afterwards, we executed cabal build and dist-newstyle had the following structure:

`dist-newstyle` after `cabal build --only-configure` followed by `cabal build`
> tree dist-newstyle/
dist-newstyle/
├── build
│   └── x86_64-linux
│       └── ghc-8.6.4
│           └── fprog-0.1.0.0
│               ├── build
│               │   ├── Angabe6.dyn_hi
│               │   ├── Angabe6.dyn_o
│               │   ├── Angabe6.hi
│               │   ├── Angabe6.o
│               │   ├── autogen
│               │   │   ├── cabal_macros.h
│               │   │   └── Paths_fprog.hs
│               │   ├── Lib.dyn_hi
│               │   ├── Lib.dyn_o
│               │   ├── Lib.hi
│               │   ├── libHSfprog-0.1.0.0-inplace.a
│               │   ├── libHSfprog-0.1.0.0-inplace-ghc8.6.4.so
│               │   └── Lib.o
│               ├── cache
│               │   ├── build
│               │   ├── config
│               │   └── registration
│               ├── package.conf.inplace
│               │   ├── package.cache
│               │   └── package.cache.lock
│               ├── setup-config
│               └── x
│                   └── intcode
│                       ├── build
│                       │   └── intcode
│                       │       ├── autogen
│                       │       │   ├── cabal_macros.h
│                       │       │   └── Paths_fprog.hs
│                       │       ├── intcode
│                       │       └── intcode-tmp
│                       │           ├── Main.hi
│                       │           └── Main.o
│                       ├── cache
│                       │   ├── build
│                       │   ├── config
│                       │   └── registration
│                       ├── package.conf.inplace
│                       │   ├── package.cache
│                       │   └── package.cache.lock
│                       └── setup-config
├── cache
│   ├── compiler
│   ├── config
│   ├── elaborated-plan
│   ├── improved-plan
│   ├── plan.json
│   ├── solver-plan
│   ├── source-hashes
│   └── up-to-date
├── packagedb
│   └── ghc-8.6.4
│       ├── fprog-0.1.0.0-inplace.conf
│       ├── package.cache
│       └── package.cache.lock
└── tmp

In comparison, the dist-newstyle is identical when cabal build is invoked directly without invoking cabal build --only-configure first.

I was unable to prove that the configure function in Cabal is actually called by cabal v2-build, therefore we still call the function writeAutogenFiles transitively in build.If configure is called by cabal v2-build then we could omit this call.
EDIT: removing the call to writeAutogenFiles in build seems to work as expected.

cc @jneira

@phadej
Copy link
Collaborator

phadej commented Dec 17, 2019

I fill uneasy by this. What configure and build steps do (or/and shoulddo) is not entirely clear to me, and in e.g. cabal-doctest we do generate autogen files only in build step.

So I'd assume that Paths_pkg name would be only generated in build step. No strong opinion on cabal_macros.h though.

I think we should not move generation of Paths_pkgname.hs to configure step, as we don't run preprocessors to generate .hs from .hsc for example there either.

If there's some problem to be solved, it should take into account the preprocessors.

EDIT: if we need a separate step to call preprocessors, then we should add one.

@fendor
Copy link
Collaborator Author

fendor commented Dec 17, 2019

I share your uneasienss, since I myself do not understand the big image.
So, is the suggestion to only generate the cabal_macros.h in configure and Paths_*.hs in build step?
Also, I dont know how to implement separate step, are there succinct examples of such implementations?

@phadej
Copy link
Collaborator

phadej commented Dec 17, 2019

One idea is to generate cabal_macros.h, Paths_*.hs and preprocessor files all in build, but have build split into more phases, where --only-configure is one, but so cabal-install can stop after each one. Incl. before linking executables for example. The build step is big and monolithic, so splitting it properly once would be good (IMO), at least better than trying to solve one individual use case.

EDIT: good opportunity to revisit what all the build actually does.
NOTE: usual pattern with build-type: Custom is do something before or/and after default build; we have to take that into account when thinking about phases. I.e. it might make sense to run build, but stop immediately as custom setup might do something.

@jneira
Copy link
Member

jneira commented Dec 17, 2019

maybe, as a first step of a more integral solution like the suggested one by @phadej could be activate the generation of files with a flag (the next step?) so cabal build --write-autogen-files could do the same as cabal build --only-configure plus generate the files.
NOTE: Add a flag was suggested by @nh2 in #2209, but in the configure step. I think cabal_macros.h could fit in configure but i also feel Paths*.hs belongs to build.

@phadej
Copy link
Collaborator

phadej commented Dec 17, 2019

I think cabal_macros.h could fit in configure but i also feel Paths*.hs belongs to build.

I don't disagree: most functionality of cabal_macros.h is actually in the GHC now, except the generation of CURRENT_PACKAGE_KEY, CURRENT_PACKAGE_KEY and CURRENT_PACKAGE_KEY #defines.

And the information is a bit like producing foo.h from foo.h.in where input is determined by the enrivonment (= dependencies).

@phadej
Copy link
Collaborator

phadej commented Dec 18, 2019

... however. build-type: Custom which wouldn't use recent Cabal the library, won't magically start creating cabal_macros.h in configure step. So even it it's feels that ./Setup configure could already create cabal_macros.h, it probably wouldn't.

So I would leave

configure :: (GenericPackageDescription, HookedBuildInfo)
          -> ConfigFlags -> IO LocalBuildInfo

as is, and consider generation of cabal_macros.h as a build step. Probably the first one, but nevertheless, a build step.

@fendor
Copy link
Collaborator Author

fendor commented Feb 16, 2021

We never followed this idea, so closing

@fendor fendor closed this Feb 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add feature to generate cabal_macros.h
3 participants