Make the symbol graph extraction requested by plugins more robust #3993

abertelrud · 2022-01-08T18:11:24Z

This includes reworking the SymbolGraphExtract type that so it's more suitable for being called in the context of plugins.

Motivation:

The symbol graph extraction support for plugins called into the existing SymbolGraphExtract type, but this type was more suited to use from the command line action, and this caused some bugs for its use from plugins.

This PR refactors SymbolGraphExtract to hoist the assumptions about things like destination path out into the caller, and adjusts the call sites.

Modifications

factor out assumptions and console output from SymbolGraphExtract to the caller
when used for plugins, avoid generate symbol graph information for targets other than the one asked for
use a unique symbol graph output directory for each package/target combination
add a unit test with better checking for different symbol graph generation usage
adjust call sites for the dump-symbol-graph command as well as its use from plugins
made verbose mode pass -v to dump-symbol-graph so it prints additional information too

Results

Same behavior for dump-symbol-graph except that verbose mode is now also passed on to that tool, which is in the spirit of enabling verbose more. For package plugin use, there are now separate output paths per combination of package and target, and only those symbol graphs that are asked for are emitted.

rdar://87234110&86785765

This will be nominated for inclusion in SwiftPM 5.6.

abertelrud · 2022-01-08T18:12:05Z

@swift-ci smoke test

abertelrud · 2022-01-08T18:24:17Z

@swift-ci smoke test

tomerd

few suggestions

tomerd · 2022-01-08T19:36:54Z

Sources/Commands/SymbolGraphExtract.swift

-
-            print("-- Emitting symbol graph for", target.name)
-            try runTool(args)
+        try localFileSystem.createDirectory(outputDirectory, recursive: true)


should this support abstract file system instead of assuming local?

Since the tool only works locally there didn't seem to be much point, and could be misleading since only the local file system will work.

tomerd · 2022-01-08T19:38:00Z

Tests/CommandsTests/PackageToolTests.swift

+            // Create a sample package with a library, and executable, and a plugin.
+            let packageDir = tmpPath.appending(components: "MyPackage")
+            try localFileSystem.writeFileContents(packageDir.appending(components: "Package.swift")) {
+                $0 <<< """


you can now pass string directly, o need to go through the byte stream

Great, I didn't realize. All the other tests use the stream, but I can change this one over.

There seems to be a subtle difference, where for the block-and-stream version, the parent directory of the output file is automatically created, but for the others (string, bytes, etc) it's not. I'm inclined to leave this as is just in the interesting of getting more bugs fixed and then deal with all the tests in a separate PR, possibly also modifying the other calls to at least optionally create the parent directories of the output file.

Sources/Commands/SwiftPackageTool.swift

…in the context of plugins. This includes parameterizing the output that's printed and the output path, and applying the extractor to a target at a time and not always to all the targets in a package. The original behavior for the `dump-symbol-graph` command is unmodified, but some of the specifics are hoisted into the DumpSymbolGraph command implementation rather than in the reusable SymbolGraphExtract type.

…SymbolGraphExtract: - only generate symbol graph information for the specified target - use a unique symbol graph output directory for each package/target combination Also add a unit test to check these things. rdar://87234110&86785765

- change output format to an enumeration - change verbosity to log level to avoid using the global

abertelrud · 2022-01-08T21:42:58Z

@swift-ci smoke test

abertelrud · 2022-01-08T23:42:40Z

Error is:

15:32:48 error: unspecified("toolchain is invalid: could not find swift-symbolgraph-extract at expected path /Users/buildnode/jenkins/workspace/swift-package-manager-PR-macos-smoke-test/branch-main/buildbot_incremental/swiftpm-macosx-x86_64/x86_64-apple-macosx/release/swift-symbolgraph-extract")

I'm going to have to think about the best way of fixing this one — it's only an issue during testing, but it would be better to have the tests always running rather than conditionalize on availability of this tool. It looks like UserToolchain uses the path of the Swift compiler to determine the toolchain path (which is reasonable) but during smoke tests cannot find this tool there.

It doesn't seem that we had any previous unit tests for the dump-symbol-graph functionality.

…iler that hasn't built the `swift-symbolgraph-extract` tool. There is a `SWIFT_SYMBOLGRAPH_EXTRACT` env var that could be used to point it another binary, but on platforms other than macOS we don't have an existing Xcode or anything that would be guaranteed to contain this tool. This test should still run on full builds and on self tests, where the tool is available.

abertelrud · 2022-01-09T02:00:38Z

@swift-ci smoke test

abertelrud · 2022-01-09T06:05:54Z

Error is:
15:32:48 error: unspecified("toolchain is invalid: could not find swift-symbolgraph-extract at expected path /Users/buildnode/jenkins/workspace/swift-package-manager-PR-macos-smoke-test/branch-main/buildbot_incremental/swiftpm-macosx-x86_64/x86_64-apple-macosx/release/swift-symbolgraph-extract")
I'm going to have to think about the best way of fixing this one — it's only an issue during testing, but it would be better to have the tests always running rather than conditionalize on availability of this tool. It looks like UserToolchain uses the path of the Swift compiler to determine the toolchain path (which is reasonable) but during smoke tests cannot find this tool there.

I ended up making the test conditional on the presence of the swift-symbolgraph-extract tool. There are cases in which we could find a different version of it in the installed toolchain, but as these test will find it in both the self tests and also in the full-build case, it seems reasonable to skip the test if the tool isn't available at all.

…iftlang#3993) * Rework the SymbolGraphExtract so it's more suitable for being called in the context of plugins. This includes parameterizing the output format and the output path, as well as the diagnostics that are printed, and applying the extractor to a target at a time and not always to all the targets in a package. The original behavior for the `dump-symbol-graph` command is unmodified, but some of the specifics are hoisted into the DumpSymbolGraph command implementation rather than in the reusable SymbolGraphExtract type. * Make the symbol graph generation more robust by using the refactored SymbolGraphExtract: - only generate symbol graph information for the specified target - use a unique symbol graph output directory for each package/target combination - added a unit test to check these things. * Skip the test in a bootstrap situation where we're using a Swift compiler that hasn't built the `swift-symbolgraph-extract` tool. There is a `SWIFT_SYMBOLGRAPH_EXTRACT` env var that could be used to point it another binary, but on platforms other than macOS we don't have an existing Xcode or anything that would be guaranteed to contain this tool. This test should still run on full builds and on self tests, where the tool is available. rdar://87234110&86785765 (cherry picked from commit 850c772)

) (#3998) * Rework the SymbolGraphExtract so it's more suitable for being called in the context of plugins. This includes parameterizing the output format and the output path, as well as the diagnostics that are printed, and applying the extractor to a target at a time and not always to all the targets in a package. The original behavior for the `dump-symbol-graph` command is unmodified, but some of the specifics are hoisted into the DumpSymbolGraph command implementation rather than in the reusable SymbolGraphExtract type. * Make the symbol graph generation more robust by using the refactored SymbolGraphExtract: - only generate symbol graph information for the specified target - use a unique symbol graph output directory for each package/target combination - added a unit test to check these things. * Skip the test in a bootstrap situation where we're using a Swift compiler that hasn't built the `swift-symbolgraph-extract` tool. There is a `SWIFT_SYMBOLGRAPH_EXTRACT` env var that could be used to point it another binary, but on platforms other than macOS we don't have an existing Xcode or anything that would be guaranteed to contain this tool. This test should still run on full builds and on self tests, where the tool is available. rdar://87234110&86785765 (cherry picked from commit 850c772)

abertelrud requested review from elsh, neonichu and tomerd as code owners January 8, 2022 18:11

abertelrud self-assigned this Jan 8, 2022

abertelrud added the ready Author believes the PR is ready to be merged & any feedback has been addressed label Jan 8, 2022

abertelrud force-pushed the eng/symbolication-path branch from 54cafa6 to ad13f0a Compare January 8, 2022 18:21

tomerd approved these changes Jan 8, 2022

View reviewed changes

abertelrud added 3 commits January 8, 2022 13:15

Fixes based on feedback:

9dd86bb

- change output format to an enumeration - change verbosity to log level to avoid using the global

abertelrud force-pushed the eng/symbolication-path branch from ad13f0a to 9dd86bb Compare January 8, 2022 21:41

tomerd approved these changes Jan 8, 2022

View reviewed changes

abertelrud merged commit 850c772 into swiftlang:main Jan 9, 2022

abertelrud deleted the eng/symbolication-path branch January 9, 2022 06:07

abertelrud mentioned this pull request Jan 9, 2022

[5.6] Make the symbol graph extraction requested by plugins more robust (#3993) #3998

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make the symbol graph extraction requested by plugins more robust #3993

Make the symbol graph extraction requested by plugins more robust #3993

Uh oh!

abertelrud commented Jan 8, 2022 •

edited

Loading

Uh oh!

abertelrud commented Jan 8, 2022

Uh oh!

abertelrud commented Jan 8, 2022

Uh oh!

tomerd left a comment

Uh oh!

tomerd Jan 8, 2022

Uh oh!

abertelrud Jan 8, 2022

Uh oh!

tomerd Jan 8, 2022

Uh oh!

abertelrud Jan 8, 2022

Uh oh!

abertelrud Jan 8, 2022

Uh oh!

Uh oh!

Uh oh!

abertelrud commented Jan 8, 2022

Uh oh!

abertelrud commented Jan 8, 2022 •

edited

Loading

Uh oh!

abertelrud commented Jan 9, 2022

Uh oh!

abertelrud commented Jan 9, 2022

Uh oh!

Uh oh!

Make the symbol graph extraction requested by plugins more robust #3993

Make the symbol graph extraction requested by plugins more robust #3993

Uh oh!

Conversation

abertelrud commented Jan 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation:

Modifications

Results

Uh oh!

abertelrud commented Jan 8, 2022

Uh oh!

abertelrud commented Jan 8, 2022

Uh oh!

tomerd left a comment

Choose a reason for hiding this comment

Uh oh!

tomerd Jan 8, 2022

Choose a reason for hiding this comment

Uh oh!

abertelrud Jan 8, 2022

Choose a reason for hiding this comment

Uh oh!

tomerd Jan 8, 2022

Choose a reason for hiding this comment

Uh oh!

abertelrud Jan 8, 2022

Choose a reason for hiding this comment

Uh oh!

abertelrud Jan 8, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

abertelrud commented Jan 8, 2022

Uh oh!

abertelrud commented Jan 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abertelrud commented Jan 9, 2022

Uh oh!

abertelrud commented Jan 9, 2022

Uh oh!

Uh oh!

abertelrud commented Jan 8, 2022 •

edited

Loading

abertelrud commented Jan 8, 2022 •

edited

Loading