Clean ups to the swift-syntax-dev-utils and Package.swift #2200
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kimdv
approved these changes
Sep 14, 2023
Comment on lines
518
to
519
| ValidationFailure(node: .editorPlaceholderPattern, message: "could conform to trait 'MissingNode' but does not"), | ||
| ValidationFailure(node: .editorPlaceholderType, message: "could conform to trait 'MissingNode' but does not"), |
There was a problem hiding this comment.
After rebase those should not be needed anymore
…the top and add documentation to them Also, make some easy clean-ups: - Don’t set `SWIFTSYNTAX_NO_OSLOG_DEPENDENCY` because it’s not needed - Use the Swift settings of a target also for the corresponding test target. That’s what we already did for SwiftParser and SwiftParserTests. We should be consistent here.
`invokeSwiftPM` already sets `--verbose`. No need to pass it again in the `additionalArguments`.
ahoppen
force-pushed
the
ahoppen/build-cleanup
branch
from
24bf776 to
77819b7
Compare
|
@swift-ci Please test |
bnbarham
reviewed
Sep 18, 2023
Package.swift
Outdated
Comment on lines
17
to
18
| /// - Enables raw syntax validation (ie. implies `SWIFTSYNTAX_ENABLE_RAWSYNTAX_VALIDATION`) | ||
| /// - Enables alternate token introspection (ie. implies `SWIFTPARSER_ENABLE_ALTERNATE_TOKEN_INTROSPECTION`) |
There was a problem hiding this comment.
As written, neither of these are true. Both of these are only enabled if the environment variable is set.
There was a problem hiding this comment.
Oh, damn. That was still from an earlier version of the PR.
| @@ -153,10 +181,11 @@ let package = Package( | |||
|
|
|||
| .testTarget( | |||
| name: "SwiftSyntaxTest", | |||
| dependencies: ["_SwiftSyntaxTestSupport", "SwiftSyntax", "SwiftSyntaxBuilder"] | |||
| dependencies: ["_SwiftSyntaxTestSupport", "SwiftSyntax", "SwiftSyntaxBuilder"], | |||
| swiftSettings: swiftSyntaxSwiftSettings | |||
There was a problem hiding this comment.
Out of interest, was this causing rebuilds of the other targets?
There was a problem hiding this comment.
Nope, this PR is pure cleanup. Fixing all the rebuilds is
- Disable testable imports when testing swift-format swift-format#624
- Don’t set
SWIFT_BUILD_SCRIPT_ENVIRONMENTwhen building swift-format swift-format#630 - Don’t set
SWIFT_BUILD_SCRIPT_ENVIRONMENTwhen building swift-format swift-stress-tester#251 - [build] Build the SourceKit stress tester and SwiftEvolve next to each other swift#68562
- Pass
--verbosetoswift buildswift-stress-tester#252
This flag wasn’t used anywhere.
ahoppen
force-pushed
the
ahoppen/build-cleanup
branch
from
77819b7 to
e2f928d
Compare
|
@swift-ci Please test |
bnbarham
approved these changes
Sep 18, 2023
|
@swift-ci Please test |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SWIFTSYNTAX_NO_OSLOG_DEPENDENCYbecause it’s not needed--verbosetwice when invoking SwiftPM fromswift-syntax-dev-utilsinvokeSwiftPMalready sets--verbose. No need to pass it again in theadditionalArguments.--disable-sandbox. This flag wasn’t used anywhere.