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

Adds virtual-modules to handle cases like GHC.Prim from ghc-prim. #4875

Merged
merged 2 commits into from
Nov 11, 2017

Conversation

angerman
Copy link
Collaborator

@angerman angerman commented Nov 8, 2017

This used to be done in ghc-cabal. With the long term goal of getting rid of ghc-cabal eventually, it's functionality needs to end up in cabal.

While this is a custom hack, it is restricted to the GHC module only. A more generic solution would be to add virtual-modules I suppose. GHC does custom handling for GHC.Prim, and there is no support for virtual-modules in the package config file either as far as I can see. As such this seems to be the least invasive solution.

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!

@angerman
Copy link
Collaborator Author

angerman commented Nov 8, 2017

How much documentation would this need?

@hvr
Copy link
Member

hvr commented Nov 8, 2017

I'm a bit conflicted about this... yet another magic undocumented logic... what if I have a custom package which happens to define a module named GHC.Prim for fun... is "GHC.Prim" now considered to be a reserved module name nobody can use? :-)

@23Skidoo
Copy link
Member

23Skidoo commented Nov 8, 2017

Hmm, so now I can't make a package that has a module named GHC.Prim? Can you perhaps make this hack only apply to the base package (and add a "MASSIVE HACK" disclaimer while you're at it)? Though virtual-modules wouldn't be that hard to add, I think.

@hvr
Copy link
Member

hvr commented Nov 8, 2017

Btw, to makes things more interesting, ghc-prim has

flag include-ghc-prim
    Description: Include GHC.Prim in exposed-modules
    default: False

library
    if flag(include-ghc-prim)
        exposed-modules: GHC.Prim

@angerman
Copy link
Collaborator Author

angerman commented Nov 8, 2017

I'm not terribly happy with this either. For virtual-modules, I'm just not sure how much use that would have outside of this specific case.

I therefore plan to go forward with a guard on the base package, and an additional MASSIVE HACK annotation.

@23Skidoo
Copy link
Member

23Skidoo commented Nov 8, 2017

For virtual-modules, I'm just not sure how much use that would have outside of this specific case.

Most likely none, but at least you won't need to patch Cabal again if base gets a new virtual module.

@angerman
Copy link
Collaborator Author

angerman commented Nov 8, 2017

Most likely none, but at least you won't need to patch Cabal again if base gets a new virtual module.

I believe GHC.Prim to be rather special and unique. Any additional virtual module, would require additional special casing in GHC as well.

@angerman angerman changed the title Adds custom handling of the GHC.Prim module during copy Adds virtual-modules to handle cases like GHC.Prim from ghc-prim. Nov 10, 2017
GHC's `ghc-prim` package has an `exposed-module` for which there exists no source code. As such it can't be built and has no interface file.  With the goal of getting rid of hacks in GHC `ghc-prim`s cabal file, which does the following:

```
flag include-ghc-prim
    Description: Include GHC.Prim in exposed-modules
    default: False
[...]
    if flag(include-ghc-prim)
        exposed-modules: GHC.Prim
```

and has this explaination in the build system:

```
# ----------------------------------------
# Special magic for the ghc-prim package

# We want the ghc-prim package to include the GHC.Prim module when it
# is registered, but not when it is built, because GHC.Prim is not a
# real source module, it is built-in to GHC.  The old build system did
# this using Setup.hs, but we can't do that here, so we have a flag to
# enable GHC.Prim in the .cabal file (so that the ghc-prim package
# remains compatible with the old build system for the time being).
# GHC.Prim module in the ghc-prim package with a flag:
#
libraries/ghc-prim_CONFIGURE_OPTS += --flag=include-ghc-prim

# And then we strip it out again before building the package:
define libraries/ghc-prim_PACKAGE_MAGIC
libraries/ghc-prim_dist-install_MODULES := $$(filter-out GHC.Prim,$$(libraries/ghc-prim_dist-install_MODULES))
endef
```

The addition of the `virtual-modules` has thus the effect of a module that is assumed to exist
but not tried to be built or listed in any of the module files.  It is however part of the
`exposed-modules` when registered into the package database
Copy link
Member

@23Skidoo 23Skidoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@angerman
Copy link
Collaborator Author

I'll do some more test, and merge if nothing falls through the cracks.

@angerman
Copy link
Collaborator Author

Seems to work for me as expected.

@angerman angerman merged commit 09dd45e into haskell:master Nov 11, 2017
@angerman angerman deleted the feature/ghc-prim branch November 11, 2017 06:12
angerman added a commit to zw3rk/cabal that referenced this pull request Nov 15, 2017
…res, extra-library-flavours, and virtual-modules.

As pointed out by @hvr, haskell#4857, haskell#4875 did not contain the necessary "check" logic. This PR tries to address this shortcoming.
@angerman angerman mentioned this pull request Nov 15, 2017
4 tasks
bgamari pushed a commit to ghc/ghc that referenced this pull request Nov 18, 2017
Stop the GHC.Prim madness with `virtual-module` support from cabal.
Needs haskell/cabal#4875.

Bumps submodule libraries/Cabal to include the necessary logic for `virtual-module`.

Reviewers: bgamari

Reviewed By: bgamari

Subscribers: rwbarton, thomie

Differential Revision: https://phabricator.haskell.org/D4179
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants