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

Add field for setting MacOS framework search path (fixes #182). #3158

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@komadori
Copy link
Collaborator

komadori commented Feb 16, 2016

This adds a new field for specifying the framework search path on MacOS akin to include-dirs and extra-lib-dirs. It translates into specifying the -framework-path option on the GHC command line. The name was chosen to match the pre-existing field in InstalledPackageInfo.

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Feb 16, 2016

Code looks fine, though I'm not sure whether specifying this info in .cabal files is a good idea. But you're right that there's already precedent.

Also, since I don't use OS X, would be nice to get some input from active OS X users.

/cc @dcoutts
/cc @cartazio

@komadori

This comment has been minimized.

Copy link
Collaborator

komadori commented Feb 17, 2016

Well, for example, if I wanted to build against Qt 5.4 specifically then I know that the Qt SDK installer will have installed the frameworks in /opt/Qt/5.4/clang_64/lib and I could put that in my cabal file. If I just wanted to build against the latest version of Qt then there isn't a path I could hard code. I don't know how other libraries fare with the consistency of their installed locations. @mietek suggests in the associated bug that brew installs packages to fixed locations, but these also appear to include the version number in their path. OTOH, if you can't specify the framework path, then at present AFAIK you're have to copy your frameworks into the Xcode bundle, which is somewhat unfriendly to do.

This evidence is a little weak and suggests it's less widely applicable than the other xxxDirs, but not exactly a grave misstep. Of course, I'm biased in favour because this would nicely and orthogonally solve the problem that I'm having right now.

My actual use case is that I have Setup.hs hooks which rewrite fields like includeDirs and extraLibDirs to point to the correct location based on custom logic. I inject the -framework-path into the GHC options too, except that doesn't work for shared library builds because there isn't any way of getting into the ghcSharedLinkArgs construction in Distribution.Simple.GHC. So, an alternative for me, if this were rejected, would be to provide some other way of passing options into ghcSharedLinkArgs.

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Feb 17, 2016

Thanks for the explanation. I'm not opposed to including this, just want to hear more opinions.

So, an alternative for me, if this were rejected, would be to provide some other way of passing options into ghcSharedLinkArgs.

Well, we already have ghc-shared-options, maybe those should be passed to ghcSharedLinkArgs as well?

@komadori

This comment has been minimized.

Copy link
Collaborator

komadori commented Feb 17, 2016

Ah, I have a better example now. I installed Brew last night and set it building R, and it placed R.framework in /usr/local/Frameworks. So, in that case a developer might like to set that path with framework-dirs.

On the subject of sharedOptions, I presumed it was used for something else, but a quick GitHub search (on my phone :-P) doesn't actually show any use sites. If that's corect, then it does seem that it's actually missing from ghcSharedLinkArgs.

23Skidoo added a commit to 23Skidoo/cabal that referenced this pull request Feb 18, 2016

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Feb 18, 2016

If you search for hcSharedOptions, you'll see that they're used when compiling Haskell sources to object files. I grepped through all .cabal files on Hackage, and it seems like the only thing ghc-shared-options are used for is -prof -auto-all (which is actually wrong, see #3161). So using them during the linking phase should be fine (implemented in #3162).

Speaking of your patch, you also need to add a cabal-version check to Distribution.PackageDescription.Check.checkCabalVersion. I think I'll merge this PR once you add this change.

@dcoutts

This comment has been minimized.

Copy link
Member

dcoutts commented Feb 18, 2016

So indeed it is a bit dubious that we have in .cabal files the "extra-lib-dirs" and "include-dirs" (though include-dirs can refer to local as well as system locations), since it's very often the case that we don't know where these dirs are in advance, only when we get to the system. But sometimes it makes sense. But of course we also have corresponding options that the package builder can specify, ie extra-lib-dirs extra-include-dirs (either on the command line or in config files).

So my view is that as with lib dirs and include dirs in the .cabal file it's ok to have framework dirs (and in principle also windows assembly equiv) but we must also have a way for the package builder to specify these too, ie an --extra-framework-dirs flag for configure (and thus by extension a extra-framework-dirs field in the config files).

@dcoutts

This comment has been minimized.

Copy link
Member

dcoutts commented Feb 18, 2016

I see I said a similar thing before: #182 (comment)

@dcoutts

This comment has been minimized.

Copy link
Member

dcoutts commented Feb 18, 2016

So to be clear: this patch looks fine but we also must have a corresponding --extra-framework-dirs flag for configure. It should be easy to add by following the pattern for --extra-lib-dirs and --extra-include-dirs.

@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Feb 19, 2016

@komadori And please also add a line to Cabal/changelog.

23Skidoo added a commit to 23Skidoo/cabal that referenced this pull request Feb 20, 2016

Address review comments in haskell#3158.
* Renamed to 'extra-framework-dirs'.
* Added 'extra-framework-dirs' to ConfigFlags.
* Added some checks.
* Updated changelog.

23Skidoo added a commit to 23Skidoo/cabal that referenced this pull request Feb 20, 2016

Address review comments in haskell#3158.
* Renamed to 'extra-framework-dirs'.
* Added 'extra-framework-dirs' to ConfigFlags.
* Added some checks.
* Updated changelog.
@23Skidoo

This comment has been minimized.

Copy link
Member

23Skidoo commented Feb 20, 2016

Closing in favour of #3176.

@23Skidoo 23Skidoo closed this Feb 20, 2016

23Skidoo added a commit to 23Skidoo/cabal that referenced this pull request Feb 20, 2016

Address review comments in haskell#3158.
* Renamed to 'extra-framework-dirs'.
* Added 'extra-framework-dirs' to ConfigFlags.
* Added some checks.
* Updated changelog.

garetxe added a commit to garetxe/cabal that referenced this pull request Mar 5, 2016

garetxe added a commit to garetxe/cabal that referenced this pull request Mar 5, 2016

Address review comments in haskell#3158.
* Renamed to 'extra-framework-dirs'.
* Added 'extra-framework-dirs' to ConfigFlags.
* Added some checks.
* Updated changelog.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment