Skip to content

Implement fileAffectsSwiftOrClangBuildSettings with the logic from LLBuildManifestBuilder #7699

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.

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

Merged
merged 1 commit into from
Jun 25, 2024

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Jun 22, 2024

Instead of inspecting file extensions to decide whether a file might affect build settings, check which files are in directories that are affecting compilation. This is the same logic that LLBuildManifestBuilder uses and should be more stable. In particular, this stops us from reloading the package manifest in SourceKit-LSP when a header file is written to a .build directory.

rdar://128573306

@ahoppen
Copy link
Member Author

ahoppen commented Jun 22, 2024

swiftlang/sourcekit-lsp#1508

@swift-ci Please test

@ahoppen ahoppen force-pushed the file-affects-build-settings branch 2 times, most recently from 49dfe0d to 26b8e0b Compare June 24, 2024 14:31
@MaxDesiatov
Copy link
Contributor

@swift-ci test

@MaxDesiatov
Copy link
Contributor

@swift-ci test windows

@ahoppen
Copy link
Member Author

ahoppen commented Jun 24, 2024

swiftlang/sourcekit-lsp#1508

@swift-ci Please test

@@ -654,6 +662,34 @@ public class BuildPlan: SPMBuildCore.BuildPlan {
}
return try description.symbolGraphExtractArguments()
}

/// Returns the files and directories that affect the build process of this build plan.
package var inputs: [Input] {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure whether this is relevant but - traits are currently not considered by package structure command, is it possible for them to affect what files are included in the build and subsequently influence fileAffectsSwiftOrClangBuildSettings?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Traits aren't affecting what files are included in a single module. The same files will be part of the module just different SWIFT_ACTIVE_COMPILATION_CONDITIONS are passed. Traits do effect build planning though. The swift build/run/test commands support trait configuration arguments to change the traits of the root manifests. If the root trait changes we have to re-plan the build. This is already implemented and the trait configuration is stored in the build description.

What are the package structure commands in question here?

Copy link
Contributor

Choose a reason for hiding this comment

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

PackageStructure command determines whether anything changed in the package and would skip loading of modules graph, and rebuilding of the build plan if nothing changed if traits are not part of that consideration there is a possibility of a split-brain scenario if something started directly using buildPackageStructure() result as an indicator for a rebuild.

…`LLBuildManifestBuilder`

Instead of inspecting file extensions to decide whether a file might affect build settings, check which files are in directories that are affecting compilation. This is the same logic that `LLBuildManifestBuilder` uses and should be more stable. In particular, this stops us from reloading the package manifest in SourceKit-LSP when a header file is written to a `.build` directory.

rdar://128573306
@ahoppen ahoppen force-pushed the file-affects-build-settings branch from 26b8e0b to 99d92ca Compare June 24, 2024 20:32
@ahoppen
Copy link
Member Author

ahoppen commented Jun 24, 2024

swiftlang/sourcekit-lsp#1508

@swift-ci Please test

@MaxDesiatov
Copy link
Contributor

swiftlang/sourcekit-lsp#1508

@swift-ci test windows

ahoppen added a commit that referenced this pull request Jun 26, 2024
… from `LLBuildManifestBuilder` (#7709)

- **Explanation**: Instead of inspecting file extensions to decide
whether a file might affect build settings, check which files are in
directories that are affecting compilation. This is the same logic that
`LLBuildManifestBuilder` uses and should be more stable. In particular,
this stops us from reloading the package manifest in SourceKit-LSP when
a header file is written to a `.build` directory.
- **Scope**: Only affects SourceKit-LSP
- **Risk**: Low, only affects SourceKit-LSP
- **Testing**: Adding a SourceKit-LSP test case in
swiftlang/sourcekit-lsp#1512
- **Issue**: rdar://128573306
- **Reviewer**: @MaxDesiatov @xedin on
#7699
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants