Skip to content

Partial migration of Usage.md - topics related to dependencies and C language modules #8709

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
May 27, 2025
Merged

Partial migration of Usage.md - topics related to dependencies and C language modules #8709

merged 10 commits into from
May 27, 2025

Conversation

heckj
Copy link
Member

@heckj heckj commented May 21, 2025 •
edited
Loading

Migrating content from documentation/Usage.md into DocC:

New articles:

  • AddingDependencies.md
  • CreatingCLanguageTargets.md
  • ResolvingPackageVersions.md
  • Dependencies/AddingSystemLibraryDependency.md
  • Dependencies/ExampleSystemLibraryPkgConfig.md
  • Dependencies/ExampleSystemLibraryWithoutPkgConfig.md
  • Dependencies/ResolvingDependencyFailures.md

And updates to Documentation.md for curating them into the collection as guides.

partial work against #8593

@heckj heckj self-assigned this May 21, 2025
@heckj heckj force-pushed the docs/migrate-usage-deps branch from cd1bf7d to d307ba6 Compare May 21, 2025 22:12
@heckj heckj marked this pull request as ready for review May 22, 2025 02:28
@heckj
Copy link
Member Author

heckj commented May 22, 2025

@swift-ci please test

owenv
owenv reviewed May 22, 2025
View reviewed changes
Copy link
Contributor

@owenv owenv left a comment

Choose a reason for hiding this comment

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

nice improvement!

Sources/PackageManagerDocs/Documentation.docc/Dependencies/AddingSystemLibraryDependency.md Outdated

#### Module Map Versioning

Version the module maps semantically, using your best judgement.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should talk about versioning the package containing the module map rather than the module map itself, which doesn't have any versioning info

Copy link
Member Author

Choose a reason for hiding this comment

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

This whole section was a real challenge to transfer forward, as I'm not sure I really understood the original intent from the legacy documentation. I'll take another pass and revising this with your suggestion in mind and see if it works a bit better.

Sources/PackageManagerDocs/Documentation.docc/Dependencies/AddingSystemLibraryDependency.md Outdated

Follow the conventions of system packagers; for example, the Debian package for `python3` is called `python3`.
In Debian, there is not a single package for python; the system packagers designed it to be installed side-by-side with other versions.
The recommended name for a module map for `python3` on a Debian system is `CPython3`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should refer to the name of the module instead of the name of the module map that defines the module

Copy link
Member Author

Choose a reason for hiding this comment

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

With your feedback and @rauhul - I rewrote this section to focus on the detail of naming based on system conventions, and highlighted the idea that you may want to reserve a module name separate from the C library module interface for an idiomatic Swift wrapper.

@rauhul
Copy link
Member

rauhul commented May 22, 2025

It would be really nice if we could read these docs rendered in context of the entire doc catalog, it would at least help me provide a better review. Is this something we could achieve with a GitHub action? (we'd love to use this swift-embedded-examples too)

heckj and others added 3 commits May 21, 2025 22:21
@heckj @owenv
Update Sources/PackageManagerDocs/Documentation.docc/AddingDependenci…
2c5310c 
…es.md

Co-authored-by: Owen Voorhees <[email protected]>
@heckj @owenv
Update Sources/PackageManagerDocs/Documentation.docc/Dependencies/Add…
1d7f64c 
…ingSystemLibraryDependency.md

Co-authored-by: Owen Voorhees <[email protected]>
@heckj @owenv
Update Sources/PackageManagerDocs/Documentation.docc/Dependencies/Add…
6998f0f 
…ingSystemLibraryDependency.md


much better, thank you!

Co-authored-by: Owen Voorhees <[email protected]>
@heckj
Copy link
Member Author

heckj commented May 22, 2025

@rauhul I agree completely, although I don't currently know a great structure to provide that — even with GH actions. The core difficulty being that viewing the content requires having the content hosted (even if ephemerally) somewhere. I can envision a mechanism that leverages GH pages to do this, but that has some notable challenges in that it doesn't support multiple revisions or parallel work very well, and it's hard to make it clear it's a preview, and not "formal" documentation. (I've done just that for some one-off scenarios, but not as a recurring PR review mechanism)

In the meantime, the technique that I'm using (it's far more manual) to review these kinds of PRs involves checking out the PR locally, and using docc to preview the content locally. An example (assuming you have the gh CLI tooling installed) to do that:

gh pr checkout 8709
swift package --disable-sandbox preview-documentation --target PackageManagerDocs

In other open-source projects I've worked on in the past (Kubernetes) the project had explicit CI capabilities that included hosting HTML content through Netlify in an ephemeral way that could be referenced from the PR to view the resulting updates in HTML. I'm familiar enough with DocC to help set that up, but it would involve a lot more coordination (as well as resources and likely funds to be allocated) to enable that kind of functionality.

@heckj
Copy link
Member Author

heckj commented May 23, 2025

@swift-ci please test

owenv
owenv reviewed May 23, 2025
View reviewed changes
@heckj
Copy link
Member Author

heckj commented May 23, 2025

@swift-ci please test

2 similar comments
@bripeticca
Copy link
Contributor

@swift-ci please test

@bripeticca
Copy link
Contributor

@swift-ci please test

@heckj heckj merged commit 3eac931 into main May 27, 2025
6 checks passed
@heckj heckj deleted the docs/migrate-usage-deps branch May 27, 2025 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@owenv owenv owenv approved these changes

@jakepetroules jakepetroules Awaiting requested review from jakepetroules jakepetroules is a code owner

@dschaefer2 dschaefer2 Awaiting requested review from dschaefer2 dschaefer2 is a code owner

@shawnhyam shawnhyam Awaiting requested review from shawnhyam shawnhyam is a code owner

@bripeticca bripeticca Awaiting requested review from bripeticca bripeticca is a code owner

@plemarquand plemarquand Awaiting requested review from plemarquand plemarquand is a code owner

@bkhouri bkhouri Awaiting requested review from bkhouri bkhouri is a code owner

@cmcgee1024 cmcgee1024 Awaiting requested review from cmcgee1024 cmcgee1024 is a code owner

@daveyc123 daveyc123 Awaiting requested review from daveyc123 daveyc123 is a code owner

Assignees

@heckj heckj

Labels
documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@heckj @rauhul @bripeticca @owenv