-
Notifications
You must be signed in to change notification settings - Fork 103
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
Fix #355 Add support for includes
#587
Conversation
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.
This looks good, just one question:
https://cabal.readthedocs.io/en/stable/cabal-package-description-file.html#pkg-field-includes says that this can be used for "both header files that are already installed on the system and to those coming with the package to be installed".
@mpilgrem for includes that come with the package, did you try if cabal
includes those into the tarbal? Or does cabal
require you to also list them under extra-source-files
?
If it's the latter, then we would need to address this somehow to be consistent with don't repeat yourself.
@sol, as far as I can tell:
EDIT3: *** As a complication, So, perhaps Hpack does need two keys:
and, presumably, some form of de-duplication so that a file specified under Hpack's EDIT: To add to the complication, if you want to use the files listed under Cabal's
EDIT2: The Cabal User Guide also has:
If 'should be' is to be read as 'must', then perhaps Hpack could assume that all absolute file paths are system headers and all relative paths are to be included in |
Follows the pattern of existing `install-includes`.
Rebasing on |
Yes, that's what I was hoping for. However, giving it more thought, I'm not sure if this is a sound assumption. My naïve assumption would be that a relative path actually can refer to a system include depending on what I still imagine that an algorithm could be, for each relative file name listed under
If there are multiple hits, we add all of them, and leave resolving of precedence to One downside of this approach is that it's complicated to implement, requiring similar code as what we have for module inference. |
@sol, I agree that it is complicated to implement. I am happy to give it a go, but it could take me some time. Could you tolerate a step-by-step approach? That is:
|
Taking a step back, how crucial is this feature? The Cabal documentation says that they are used for So what I kind of assume this is actually doing is pass Where |
Actually, I think I was wrong on the "complicated". I think you can just take the total sum of all
Great 😊
|
@sol, I had been assuming - perhaps incorrectly - that it was needed whenever a Haskell package used C code. My 'template' had been the Cabal file for |
Definitely not needed, see e.g. https://github.com/sol/simdutf-haskell/blob/main/package.yaml#L15
I am gonna look at it |
Here is another example of a package using |
I have seen something that implied to me that |
I don't think so, that is what |
Looking at ghc/packages-Win32@2a56e45#diff-851be7da3fa86b5b2d90284d7440f3d15f9749698c2c14d02013641124050943R100 It's not a apparent to me what the benefit of |
Again, it's not a parent to me what @RyanGlScott do you know for reason |
EDIT: I am going to further edit this post as I flesh out my understanding of the history. What I understand so far:
|
From what I have tried, I guess somebody has to go to the Cabal source and dig up the truth 😅 |
I'm not sure I know enough to say for sure. Ultimately, the code in |
@sol, having worked through the history above, the
|
Follows the pattern of existing
install-includes
. Fixes:includes
property? #355EDIT: A test added, again following
install-includes
.