[docs] Requiring System Libraries with .systemLibrary targets #6634

natikgadzhi · 2023-06-03T13:11:01Z

Updates the usage documentation around requiring system libraries.

Motivation:

I was getting started with Swift package manager, and looking to get a bit more experience using it — and stumbled on #6121, thought I'd play around and try to clean it up. Fixes #6121.

The existing documentation on main worked, but following it emitted a build-time warning about wrapping system libraries in a target, instead of it's own package.

Modifications

I've tried to make things work without warnings, tested the .systemLibrary approach, and documented my steps.

clibgit example
cjpeg example

Result:

No functional changes to the package manager behavior — just the documentation change. After the change, one should be able to follow the docs step by step, and it should work without any build warnings.

Documentation/Usage.md

tomerd · 2023-06-06T21:58:37Z

Documentation/Usage.md

+    targets: [
+        // Targets are the basic building blocks of a package, defining a module or a test suite.
+        // Targets can depend on other targets in this package and products from dependencies.
+        .executableTarget(


it may be easier to understand if we do this in two steps, first introduce the "Clibgit" target, then show how to use it in the executable target. in other words, I suggest we split this example into two snippets, and build the knowledge in layers

It did feel a little bit overwhelming when I tried to reproduce the steps, so I agree. Rewritten that in two steps, and cleaned up the pkg-config mention as well.

Documentation/Usage.md

tomerd · 2023-06-06T22:00:40Z

Documentation/Usage.md

+by running the following:
+
+    example$ pkg-config --cflags libgit2
+    -I/opt/homebrew/Cellar/libgit2/1.6.4/include


I am not sure what this means. can you elaborate what this is about?

Moving this into the note about system paths, where it makes more sense. My point was that on most systems, for most libraries, running pkg-config --cflags {library-name} will get you the inclide and lib paths.

Documentation/Usage.md

tomerd · 2023-06-06T22:02:52Z

Documentation/Usage.md

+            dependencies: ["CJPEG"],
+            path: "Sources"
+            ),
+        .systemLibrary(


ditto re. splitting the example into two steps

After splitting the first one, I'm not so sure if we should split this one as well. It would add another blob of code, but since the second example builds up on the first one, I think the concept should be clear even without splitting.

But, happy to split if you'd prefer it in two steps here as well!

tomerd · 2023-06-06T22:04:15Z

thanks so much for helping improve this under-documented area @natikgadzhi, left some comments.

cc @MaxDesiatov @neonichu

natikgadzhi · 2023-06-07T20:04:39Z

Hey @tomerd, thank you for the review! I'll clean these up later tonight.

As the particular examples get lengthy and contain snippets of code and example commands, I wonder if after this PR, it would make sense to repackage common usage scenarios as docc tutorials? There's some documentation in docc format here, it seems. If there's interest, I would take on that work, and learn a bit more about SwiftPM in the process.

tomerd · 2023-06-07T22:54:27Z

hi @natikgadzhi I think that is indeed worth considering, but we currently do not have the automation in place to publish such. we are definitely planning to address this at some point in the future tho.

natikgadzhi · 2023-06-08T11:02:42Z

@tomerd, took another pass on the feedback, and addressed most of it, commented on the rest. Thank you for a thorough read-through and review! How does it look to you now?

I'll play around with the idea of tutorials and see how far I can get. If anything, that should help me understand SwiftPM and docc better. If that ends up being useful to someone else — great, if not — no worries, I'm doing that to learn.

MaxDesiatov · 2023-06-10T21:14:32Z

@swift-ci smoke test

Documentation/Usage.md

Co-authored-by: Max Desiatov <[email protected]>

Documentation/Usage.md

MaxDesiatov

Overall LGTM, just a couple more nits

Co-authored-by: Max Desiatov <[email protected]>

MaxDesiatov

Thanks!

MaxDesiatov · 2023-06-12T15:49:53Z

@swift-ci smoke test

MaxDesiatov · 2023-06-12T15:50:17Z

@swift-ci test windows

MaxDesiatov · 2023-06-12T15:52:02Z

@swift-ci smoke test windows

natikgadzhi · 2023-06-12T15:52:11Z

@MaxDesiatov, thank you for spending your time on the PR! I know it's a small contribution, and I hope that as I learn the ropes, it'll be worth your time.

tomerd · 2023-06-12T21:47:46Z

Documentation/Usage.md


    example$ swift build -Xlinker -L/usr/local/lib/

-Create a `module.modulemap` file so it consists of the following:
+Next, create a directory `Sources/Clibgit` in your `example` project, and
+add a `module.modulemap` to it:

    module Clibgit [system] {
      header "/usr/local/include/git2.h"


there is a somewhat better way to do this, namely to create you own header file, which then include the required one with <header> so that no path hardcoding is required. eg: https://github.com/apple/swift-package-manager/tree/main/Sources/SPMSQLite3

Sorry, long flight — back in PST now.

Ah, that's a TIL for me. So it looks like:

You can, and probably should, add a header file in Sources/{LibName}/{LibName.h} that just includes {LibName.h} and/or other required headers.

The #include will resolve with the compiler environment, so it should pick up the headers correctly, and thus you don't have to write the absolute header path in the module.modulemap.

Right?

yup afaik. @neonichu to keep me honest here

yah I believe that is correct

Co-authored-by: Max Desiatov <[email protected]>

natikgadzhi · 2023-06-15T20:16:56Z

@neonichu @tomerd alright, done: f71f92e

I've double-checked that the example project builds this way in SwiftPM 5.8. Haven't extensively tested other platforms, though.

Considered adjusting the CJPEG example as well, but it's a bit different. On macOS, Homebrew does not link the headers (and libraries) for it, so just adding #include <jpeglib.h> would not work, and you'd still have to use an absolute path. So, I left the path in module.modulemap as an example.

Mergeable?

natikgadzhi · 2023-06-15T20:40:33Z

@swift-ci please smoke test

natikgadzhi · 2023-06-15T20:40:42Z

@swift-ci test windows

tomerd · 2023-06-15T20:52:32Z

@swift-ci smoke test

compnerd · 2023-06-16T05:01:41Z

Documentation/Usage.md

@@ -146,81 +146,116 @@ following files from the libgit2 system-package are of interest:
    /usr/local/lib/libgit2.dylib      # .so on Linux
    /usr/local/include/git2.h

-Swift packages that provide module maps for system libraries are handled
-differently from regular Swift packages.
+**Note:** the system library may be located elsewhere on your system, such as


I think that we should clear up some of the text here. Windows is a support platform, and these paths are not relevant there. Additionally, pkg-config is not a commonly available utility on Windows.

Added an entry on Windows paths.

compnerd · 2023-06-16T05:01:57Z

Documentation/Usage.md

+            pkgConfig: "libgit2",
+            providers: [
+                .brew(["libgit2"]),
+                .apt(["libgit2-dev"])


Perhaps add a .nuget entry as well?

Hmmm. Should we suggest LibGit2Sharp? Swift doesn't need the .Net wrapper to build it's package, but there doesn't seem to be another minimal nuget package for it.

compnerd · 2023-06-16T05:02:36Z

Documentation/Usage.md

-search paths for the system library. Note: If you don't want to use the `pkgConfig`
-parameter you can pass the path of a directory containing the library using the
-`-L` flag in commandline when building your app:
+**Note:** If you don't want to use the `pkgConfig` parameter you can pass the


pkgConfig should be elided for Windows only packages as pkg-config is not expected to be available.

Added that to the note.

compnerd · 2023-06-16T05:03:08Z

Documentation/Usage.md

+
+```c
+# git2.h
+#pragma once


Should we recommend include guards instead?

I honestly don't have enough experience with C, so I don't know if using #pragma once is perfectly fine, or a bad taste. My understanding is that using guards is guaranteed to work anywhere, and using #pragma once is shorter, but might not work in certain environments?

compnerd · 2023-06-16T05:04:30Z

Documentation/Usage.md


    module CJPEG [system] {
        header "shim.h"
-        header "/usr/local/include/jpeglib.h"
+        header "/usr/local/opt/jpeg/include/jpeglib.h"


Should we recommend something with a VFS example? That would make the majority of the code more cross-platform friendly.

For this and all your other comments, thank you! I'll catch up and clean things up a bit later today (PST) and ping you when it's all updated.

natikgadzhi · 2023-06-19T23:53:23Z

@compnerd, I cleaned up some text and recommendations around Windows builds. But I don't feel confident re: VFS paths example and guards vs pragma once.

I don't have a strong opinion that we should not recommend guards — I just don't have enough experience to have an opinion yet.

Same with VFS — I don't have enough experience to suggest where / when to use them, and what a good example might look like.

I want to see this little piece of work through. Can you please help me polish those areas? I'm open to suggestions, or perhaps we should merge this pull request, and I should get myself a Windows VM, and try and work through all the documentation on Windows, see what works, what does not, and make another pull request with any cleanup necessary?

tomerd · 2023-06-20T19:55:59Z

I think this is a big improvement the previous version and we should merge it.

@compnerd the previous version did not address windows at all, so I think we can tackle that a separate PR

tomerd · 2023-06-20T19:56:22Z

@swift-ci smoke test

tomerd · 2023-06-20T21:41:52Z

@swift-ci smoke test windows

natikgadzhi added 2 commits June 3, 2023 17:03

[docs] requiring system libraries with targets: 1/2

a5e3981

[docs] requiring system libraries with targets: 2/2

498e1b2

natikgadzhi marked this pull request as ready for review June 3, 2023 13:31

natikgadzhi requested review from abertelrud, neonichu, tomerd and MaxDesiatov as code owners June 3, 2023 13:31

natikgadzhi commented Jun 3, 2023

View reviewed changes

Documentation/Usage.md Show resolved Hide resolved

tomerd reviewed Jun 6, 2023

View reviewed changes