Foreign Library Stanza #518

ropwareJB · 2022-07-22T05:47:20Z

[Y ] Did you add a new feature to hpack?
- [Y] Did you add an acceptance test for that new feature to test/EndToEndSpec.hs?

Addresses #258.

Adds the following syntax to support foreign / shared libraries so we dont need to use the verbatim block anymore:

foreign-library:
  type: native-shared
  source-dirs: driver
  options:
    - standalone
  dependencies:
    - hpack

mpilgrem · 2023-06-18T11:23:04Z

I do not know why the CI is failing for Windows/'system' GHC (being GHC 9.6.2). I logged https://gitlab.haskell.org/ghc/ghc/-/issues/23533 as it seems to be GHC-related.

Locally, I can build and test (stack test) the Hpack respository at commit 36c44de with Stack configuration:

resolver: nightly-2023-06-18 # GHC 9.4.5
compiler: ghc-9.6.2

extra-deps:
# GHC 9.6.2 boot libaries
- Cabal-3.10.1.0
- Cabal-syntax-3.10.1.0
- directory-1.3.8.1
- time-1.12.2
- process-1.6.17.0
- Win32-2.13.3.0
# Required because the Stackage snapshot versions do not support GHC 9.6.2
- semigroupoids-6.0.0.1
- bifunctors-5.6.1

sol · 2023-06-23T05:03:04Z

Thanks @mpilgrem.

mpilgrem · 2025-03-14T20:07:50Z

I have rebased on the main branch and squashed the existing commits making up the pull request.

mpilgrem · 2025-03-14T22:35:01Z

@sol, I think this is good to merge. Do you want to review?

sol · 2025-03-17T15:00:55Z

@sol, I think this is good to merge. Do you want to review?

@mpilgrem Hold back first, I'm gonna look at it sometime soon.

sol · 2025-03-17T15:09:35Z

@mpilgrem I rebased again. But commits don't seem to be squashed yet.

mpilgrem · 2025-03-22T12:58:58Z

@sol, I've squashed but I was not sure if your review was complete subject to that?

sol

~~First, despite what I was saying earlier, I want to raise the questions "Do we really want this?" and "Is this within the scope of Hpack?"~~

For Linux I don't know, I think you usually would go the other way around and use foreign imports. Are there any plausible use cases? If it is just for experimentation, then it would probably be outside the scope of Hpack.

~~For Windows however, the GHC documentation provides a VBA example. So if people wanna move business logic from Excel to Haskell then I can sympathize with that.~~

~~Maybe that's enough as a justification?~~

EDIT: I think this can actually be useful in combination with Python and ctypes, so let's go for it.

Regarding the implementation, I think type is a no-brainer, just remove it.

options on the other hand is more complicated. From what I understand, if we drop options then you would unconditionally need to generate

if os(Windows)
  options: standalone

While this is in line with "Don't require the user to state the obvious" I'm still puzzled how we would achieve "Give the user 100% control when needed" here? I think a user will not be able to override this with verbatim. But maybe just drop it anyway and address the 100% control thing once somebody asks for it?

BTW: I kind of think that if we decide to include this feature then we should probably include a complete example in the README, similar to what the GHC documentation contains, so that a user does not need to traverse three different manuals to make sense of this. EDIT: Even simple examples are still more complicated than I would hope for, so I'm not gonna insist on this just yet.

sol · 2025-03-22T15:03:57Z

src/Hpack/Config.hs

  executableMap <- toExecutableMap packageName packageConfigExecutables packageConfigExecutable
  let
    globalVerbatim = commonOptionsVerbatim g
    globalOptions = g {commonOptionsVerbatim = Nothing}

-    executableNames = maybe [] Map.keys executableMap
+    componentNames = maybe [] Map.keys executableMap ++ maybe [] Map.keys foreignLibraryMap


I don't think this is correct.

Specifically I think the old is just fine, we don't need to update this.

sol · 2025-03-22T15:11:12Z

src/Hpack/Config.hs

@@ -611,6 +640,8 @@ data PackageConfig_ library executable = PackageConfig {
 , packageConfigCustomSetup :: Maybe CustomSetupSection
 , packageConfigLibrary :: Maybe library
 , packageConfigInternalLibraries :: Maybe (Map String library)
+, packageConfigForeignLibraries :: Maybe (Map String foreignLib)
+, packageConfigForeignLibrary :: Maybe foreignLib


So the idea here is that in addition to foreign-libraries, we also accept foreign-library where the name defaults to the package name.

I'm still undecided whether I think this is a good idea or not, but if you're gonna do that, then we need a test for it.

I guess it is actually a good idea, so let's add the test.

sol · 2025-03-22T15:14:39Z

test/EndToEndSpec.hs

+    content = [i|
+foreign-library #{name}
+  other-modules:
+      Paths_my_package


Adding the Paths_ module is a historic mistake. Maybe we shouldn't repeat that mistake for foreign libraries?

sol · 2025-03-22T15:17:12Z

README.md

@@ -151,6 +151,7 @@ library:
 | `custom-setup` | · | | See [Custom setup](#custom-setup) | | |
 | `flags`  | `flag <name>` | | Map from flag name to flag (see [Flags](#flags)) | | |
 | `library` | · | | See [Library fields](#library-fields) | | |
+| `foreign-libraries` | `foreign-library <name>` | | Map from foreign library name to a dict of [Foreign library fields](#foreign-library-fields) and global top-level fields. | | UNRELEASED |


Documentation for foreign-library is missing here.

sol · 2025-03-22T15:18:29Z

test/EndToEndSpec.hs

+        options:
+            standalone
+        |])
+


A test for foreign-library is missing here.

sol · 2025-03-22T15:34:10Z

README.md

+| `type` | . | | |
+| `lib-version-info` | . | | |
+| `options` | . | | |
+| `mod-def-file` | . | | |


Even though Cabal and GHC support this, I don't really see a use case for it. Why the hell would you add a foreign export declaration for something, only to later hide it with a mod-def-file?

My verdict is, unless we can come up with a sensible use case for this, remove it.

sol · 2025-03-22T15:36:00Z

README.md

+| `lib-version-info` | . | | |
+| `options` | . | | |
+| `mod-def-file` | . | | |
+| `other-modules` | · | All modules in `source-dirs` less `main` less any modules mentioned in `when` | |


Is the documentation actually correct?

sol · 2025-03-22T15:37:45Z

README.md

+| `options` | . | | |
+| `mod-def-file` | . | | |
+| `other-modules` | · | All modules in `source-dirs` less `main` less any modules mentioned in `when` | |
+| `generated-other-modules` | | | Added to `other-modules` and `autogen-modules`. Since `0.23.0`.


I think this was just copied from executables! At least the Since annotation is not needed anymore.

sol · 2025-03-22T15:41:02Z

src/Hpack/Config.hs

@@ -194,6 +197,7 @@ renameDependencies old new sect@Section{..} = sect {sectionDependencies = (Depen
 packageDependencies :: Package -> [(String, DependencyInfo)]
 packageDependencies Package{..} = nub . sortBy (comparing (lexicographically . fst)) $
     (concatMap deps packageExecutables)
+  ++ (concatMap deps packageForeignLibraries)


Not related to this pull request, but I think this code does not handle internal libraries correctly. I think we need a separate issue or PR for that.

sol · 2025-03-22T15:44:01Z

src/Hpack/Config.hs

@@ -176,6 +178,7 @@ package name version = Package {
 renamePackage :: String -> Package -> Package
 renamePackage name p@Package{..} = p {
    packageName = name
+  , packageForeignLibraries = fmap (renameDependencies packageName name) packageForeignLibraries


Same here, internal libraries are not handled here. If internal libraries cannot depend on the main library, then we are actually good. I'm not sure what's the situation here.

I think currently nobody is using this code, so maybe not a big issue. We could potentially deprecate it.

sol · 2025-03-22T16:50:18Z

@mpilgrem I'm probably busy tomorrow, but gonna try to look at it again on Monday.

ropwareJB mentioned this pull request Jul 22, 2022

support for foreign library artifacts (cabal 'foreign-library' stanza) #258

Open

ropwareJB marked this pull request as ready for review July 22, 2022 06:05

mpilgrem mentioned this pull request Mar 12, 2025

Implement extra-files #603

Merged

mpilgrem force-pushed the main branch 3 times, most recently from d7a929d to 1283b79 Compare March 14, 2025 20:04

mpilgrem force-pushed the main branch 2 times, most recently from a6b1a8c to 19e49dc Compare March 14, 2025 22:29

mpilgrem force-pushed the main branch from 19e49dc to dfe11d1 Compare March 14, 2025 22:46

sol self-requested a review March 17, 2025 15:01

sol force-pushed the main branch from dfe11d1 to 3d4d694 Compare March 17, 2025 15:03

Support foreign library stanza

fa3fdb3

mpilgrem force-pushed the main branch from 3d4d694 to fa3fdb3 Compare March 22, 2025 12:57

sol reviewed Mar 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Foreign Library Stanza #518

Foreign Library Stanza #518

ropwareJB commented Jul 22, 2022 •

edited

Loading

mpilgrem commented Jun 18, 2023

sol commented Jun 23, 2023

mpilgrem commented Mar 14, 2025

mpilgrem commented Mar 14, 2025

sol commented Mar 17, 2025

sol commented Mar 17, 2025

mpilgrem commented Mar 22, 2025

sol left a comment •

edited

Loading

sol Mar 22, 2025

sol Mar 26, 2025

sol Mar 22, 2025

sol Mar 26, 2025

sol Mar 22, 2025

sol Mar 22, 2025

sol Mar 22, 2025

sol Mar 22, 2025

sol Mar 22, 2025

sol Mar 22, 2025

sol Mar 22, 2025 •

edited

Loading

sol Mar 22, 2025

sol commented Mar 22, 2025

Foreign Library Stanza #518

Are you sure you want to change the base?

Foreign Library Stanza #518

Conversation

ropwareJB commented Jul 22, 2022 • edited Loading

mpilgrem commented Jun 18, 2023

sol commented Jun 23, 2023

mpilgrem commented Mar 14, 2025

mpilgrem commented Mar 14, 2025

sol commented Mar 17, 2025

sol commented Mar 17, 2025

mpilgrem commented Mar 22, 2025

sol left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sol Mar 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sol commented Mar 22, 2025

ropwareJB commented Jul 22, 2022 •

edited

Loading

sol left a comment •

edited

Loading

sol Mar 22, 2025 •

edited

Loading