Minimal viable `merge` command #821

d-ronnqvist · 2024-02-09T12:45:40Z

Bug/issue #, if applicable: rdar://114730477

Summary

This adds a first version of a docc merge command that combines documentation archives for different modules into a "combined" documentation archive.

Two notable features that are missing in this initial version are:

Merging the LMDB navigator index
Landing pages (either authored or synthesized) for the combined archive.

However, those can be added incrementally. The merge command in its current form is enough to already be usable and bring value. For example, it allows us to host our documentation for SwiftDocC and for SwiftDocCUtilities from the same repository without breaking their navigator sidebar.

Dependencies

None

Testing

Build documentation for two or more different targets.
Run docc merge /path/to/First.doccarchive /path/to/Second.doccarchive --output-path /path/to/Something.doccarchive
Use a local web server to host "Something.doccarchive".
- It should be possible to browse both localhost:PORT/documentation/first and localhost:PORT/documentation/second
  - Each target displays its own assets (if assets had the same name in both)
  - Each target displays a navigator sidebar

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

Added tests
Ran the ./bin/test script and it succeeded
Updated documentation if necessary

rdar://114730477

patshaughnessy

Looks like a great start! I think we should be a bit more careful with validations...

Can we use a temporary staging area to create the merged result in, before creating the output directory if there are no errors? This would avoid the possibility of leaving a partially merged, invalid archive in the case there are errors.
Also we should not be deleting existing directories under any circumstances. If the users of the merge command wants to delete an existing output directory, possibly because they are repeatedly merging archives over and over again for some reason, then they should use rm -rf as needed in the terminal or from a shell script. Docc should not do this for them.
Finally, let's run a validation on the paths present inside the provided archives: It's possible that two or more archives could have the same page paths for some reason. Or maybe the user provides a repeated archive path by accident on the command line. This should be a validation error I think. We don't want to be overwriting pages in an undetermined manner.

patshaughnessy · 2024-02-14T01:02:44Z

Sources/SwiftDocCUtilities/ArgumentParsing/Subcommands/Merge.swift

+            mutating func validate() throws {
+                let fileManager = Docc.Merge._fileManager
+
+                guard !archives.isEmpty else {


Does it make sense to merge just one archive? Shouldn't we also raise a validation error when archives.count == 1 ?

Yes, merging a single archive is supported. Doing so will inactivate all the external links in that archive.

patshaughnessy · 2024-02-14T01:06:49Z

Sources/SwiftDocCUtilities/ArgumentParsing/Subcommands/Merge.swift

+
+        // MARK: - Inputs & outputs
+
+        @OptionGroup(title: "Inputs & outputs")


Is this a standard idiom for using ParsableArguments ? What's the reasoning behind having this option group? Why not simply place the properties directly on Merge and get rid of InputAndOutputOptions ?

Defining option groups customizes how the help text for the command looks. It barely matters for a small command like this but for a command with many more options like docc convert it makes a big difference. I created an "Inputs & output" groups because two other commands already have one.

patshaughnessy · 2024-02-14T01:08:22Z

Sources/SwiftDocCUtilities/Action/Actions/MergeAction.swift

+            return ActionResult(didEncounterError: true, outputs: [])
+        }
+
+        try? fileManager.removeItem(at: outputURL)


Probably we should raise a validation error in Merge if the output directory already exists. Otherwise, the user might delete an entire directory structure by accident if they have a typo or other mistake in their command line.

Fixed in f98be84 with a new testErrorWhenOutputDirectoryIsNotEmpty test

patshaughnessy · 2024-02-14T01:09:32Z

Sources/SwiftDocCUtilities/Action/Actions/MergeAction.swift

+        }
+
+        try? fileManager.removeItem(at: outputURL)
+        try fileManager.copyItem(at: firstArchive, to: outputURL)


Might be a good idea to create a temporary directory here, and use that to build up the result, and then copy/move it to the final location when and if the entire merge operation is successful. Otherwise, the user would end up with a partially merged but invalid result if there was some error or exception during the merge processing.

Fixed in f98be84

patshaughnessy · 2024-02-14T01:14:00Z

Sources/SwiftDocCUtilities/Action/Actions/MergeAction.swift

+            }
+            let renderIndex = try JSONDecoder().decode(RenderIndex.self, from: jsonIndexData)
+
+            try combinedJSONIndex.merge(renderIndex)


Can we be sure that none of the archives will have overlapping paths? Should we validate this before merging the indexes like this? And before copying around all of the files above?

Probably we should have a "pre-flight" check... meaning we iterate over all the files to be merged, to be sure none of them overlap or conflict. If they do, we can report an error before actually copying any of them.

Or alternatively, as mentioned in an earlier comment, if we use a temporary directory as a staging area, then we could abort the merge if we find any conflicting paths, before creating the output path and writing anything into it.

Fixed in f98be84 with a new testErrorWhenArchivesContainOverlappingData test

Raise an error if the archives to merge has overlapping data Use a temporary directory while merging the archives

d-ronnqvist · 2024-02-15T16:20:41Z

@swift-ci please test

patshaughnessy

Great work thanks for adding all of that extra validation!

patshaughnessy · 2024-02-16T01:36:40Z

Sources/SwiftDocCUtilities/Action/Actions/MergeAction.swift

+        var archivesByTopLevelTutorialDirectory = ArchivesByDirectoryName()
+
+        // Gather all the top level /data/documentation and /data/tutorials directories to ensure that the different archives don't have overlapping data
+        for archive in archives {


Can we avoid hard-coding the list of prefixes like this? (e.g. "documentation" and "tutorials") This will lead to bugs someday if we ever add more prefixes. Maybe just search for the contents of the entire "data" directory? Or do that but exclude some known files we want to exclude?

Yes, I did it this way because I only wanted to check the if more than one archive had the same top-level module or technology. It's not intended to be supported to merge multiple archives that contain the same modules and/or technologies so checking the top-level directories is sufficient.

I updated the to code do two nested loops to gather all the unique module names and technology names inside the data directory's direct subdirectories.

d-ronnqvist · 2024-02-16T08:54:31Z

@swift-ci please test

d-ronnqvist · 2024-02-16T17:44:30Z

@swift-ci please test

creating intermediate directories as necessary

patshaughnessy · 2024-02-17T01:07:15Z

@swift-ci please test

* Move assets into subdirectory to avoid collisions when merging archives * Add basic merge command to combine documentation archives rdar://114730477 * Publish docs for SwiftDocC and SwiftDocCUtilities as a combined archive * Pass flags to generate source links for published framework docs * Raise an error if the merge output directory already exists Raise an error if the archives to merge has overlapping data Use a temporary directory while merging the archives * Consider all (future) subdirectories in the data directory * Fix to docc merge command: Copy data/documentation and data/tutorials, creating intermediate directories as necessary --------- Co-authored-by: Pat Shaughnessy <[email protected]>

This reverts commit 491d26a.

dreampiggy · 2024-11-22T11:11:03Z

Finally. Good work

AndrewVorobev · 2025-01-15T10:43:23Z

#1131
Is it possible to make somehow navigation through combined modules?

d-ronnqvist · 2025-01-15T14:43:44Z

Is it possible to make somehow navigation through combined modules?

Yes. I replied to the new issue that you filed.

dreampiggy · 2025-02-24T10:35:06Z

Does this PR, support the rendering web page (maybe this is another tool, not swift-docc core) cross references of symbols?

For example:

ModuleA:

public class A {}

ModuleB

public class B : A{}

Seems currently the simple docc merge A.doccarchive B.doccarchive -o output.doccarchive, and setup the preview at output.doccarchive/documentation, the class B and A symbol are highlighted, but can not has hyper link cross the ModuleA to ModuleB 😂

d-ronnqvist · 2025-02-24T13:29:55Z

Note: this is a one year old closed pull request.

Merging archives into a combined archives is an orthogonal feature to linking between archives. If the two archives were built with cross target links, the merged archive would display cross target links. If two archives without cross target links are merged, the combined output also won't have cross target links.

d-ronnqvist added 4 commits February 9, 2024 12:09

Move assets into subdirectory to avoid collisions when merging archives

95a24ea

Add basic merge command to combine documentation archives

8b1022f

rdar://114730477

Publish docs for SwiftDocC and SwiftDocCUtilities as a combined archive

ae3146a

Pass flags to generate source links for published framework docs

666bd3d

d-ronnqvist changed the title ~~Merge command~~ Minimal viable merge command Feb 9, 2024

Merge branch 'main' into merge-command

270122b

patshaughnessy requested changes Feb 14, 2024

View reviewed changes

Raise an error if the merge output directory already exists

f98be84

Raise an error if the archives to merge has overlapping data Use a temporary directory while merging the archives

d-ronnqvist requested a review from patshaughnessy February 15, 2024 16:20

d-ronnqvist force-pushed the merge-command branch from 902929f to f98be84 Compare February 15, 2024 16:21

Merge branch 'main' into merge-command

ed4cd89

patshaughnessy approved these changes Feb 16, 2024

View reviewed changes

d-ronnqvist added 2 commits February 16, 2024 09:37

Consider all (future) subdirectories in the data directory

7e71215

Merge branch 'main' into merge-command

e3b7ed5

Merge branch 'main' into merge-command

7aadebf

Fix to docc merge command: Copy data/documentation and data/tutorials,

15f2e05

creating intermediate directories as necessary

patshaughnessy merged commit 491d26a into swiftlang:main Feb 17, 2024

d-ronnqvist mentioned this pull request Feb 19, 2024

Update the DocC documentation script so that it works with the changes to the merge command #830

Merged

2 tasks

d-ronnqvist deleted the merge-command branch February 19, 2024 08:20

xedin added a commit that referenced this pull request Feb 19, 2024

Revert "Minimal viable merge command (#821)"

e17d4eb

This reverts commit 491d26a.

xedin mentioned this pull request Feb 19, 2024

[DNM] Revert "Minimal viable merge command" #836

Closed

mportiz08 mentioned this pull request Jul 1, 2024

Render combined navigation for merged documentation archives swiftlang/swift-docc-render#877

Merged

3 tasks

d-ronnqvist mentioned this pull request Jul 10, 2024

Inactivate known external links swiftlang/swift-docc-render#878

Merged

3 tasks

d-ronnqvist mentioned this pull request Nov 22, 2024

Any way to merge the index.json for multiple framework hosting on single Static site ? #745

Closed


		// MARK: - Inputs & outputs

		@OptionGroup(title: "Inputs & outputs")

Minimal viable merge command #821

Minimal viable merge command #821

Uh oh!

Conversation

d-ronnqvist commented Feb 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Dependencies

Testing

Checklist

Uh oh!

patshaughnessy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d-ronnqvist commented Feb 15, 2024

Uh oh!

patshaughnessy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d-ronnqvist commented Feb 16, 2024

Uh oh!

d-ronnqvist commented Feb 16, 2024

Uh oh!

patshaughnessy commented Feb 17, 2024

Uh oh!

dreampiggy commented Nov 22, 2024

Uh oh!

AndrewVorobev commented Jan 15, 2025

Uh oh!

d-ronnqvist commented Jan 15, 2025

Uh oh!

dreampiggy commented Feb 24, 2025

Uh oh!

d-ronnqvist commented Feb 24, 2025

Uh oh!

Uh oh!

Minimal viable `merge` command #821

Minimal viable `merge` command #821

d-ronnqvist commented Feb 9, 2024 •

edited

Loading