Skip to content

Use DocumentURI instead of URL in more locations #1395

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 5 commits into from
Jun 3, 2024

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Jun 1, 2024

The originator for this change was that I noticed that we weren’t able to index files that contain a + because we wouldn’t get build settings for them. This is because SwiftPMBuildSystem had a URL comparison to check if the file that we are requesting build settings for is part of the target’s source files. Now, the target’s source files represented the + as a literal + while the requested file name had it escaped as %2B, so we incorrectly inferred that the source file was not part of the target.

DocumentURI accounts for this and compares the scheme + normalized path separately. So, the solution is to work in terms of DocumentURI in SwiftPMBuildSystem.

While doing this, I also changed other files to use DocumentURI instead of URL in case they have similar issues.

@ahoppen ahoppen requested review from bnbarham and hamishknight June 1, 2024 19:58
@ahoppen ahoppen requested a review from benlangmuir as a code owner June 1, 2024 19:58
@ahoppen
Copy link
Member Author

ahoppen commented Jun 1, 2024

@swift-ci Please test

try await swiftpmBuildSystem.generateBuildGraph(allowFileSystemWrites: false)

let aPlusSomething = packageRoot.appending(components: "Sources", "lib", "a+something.swift")
print(aPlusSomething.asURL)
Copy link
Contributor

Choose a reason for hiding this comment

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

Left over from debugging?

)
)
.compilerArguments
print(arguments)
Copy link
Contributor

Choose a reason for hiding this comment

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

Left over from debugging?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I guess I should add an actual check 🙈

@ahoppen
Copy link
Member Author

ahoppen commented Jun 3, 2024

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Jun 3, 2024

@swift-ci Please test Windows

@ahoppen ahoppen enabled auto-merge June 3, 2024 17:35
let substituteFile = buildTarget.sources.sorted(by: { $0.path < $1.path }).first
{
logger.info("😅 Getting compiler arguments for \(url) using substitute file \(substituteFile)")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the emoji intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, added it during debugging so I could find this log message more easily. Intended to remove it afterwards and forgot.

@ahoppen
Copy link
Member Author

ahoppen commented Jun 3, 2024

@swift-ci Please test

@ahoppen
Copy link
Member Author

ahoppen commented Jun 3, 2024

@swift-ci Please test Windows

@ahoppen ahoppen merged commit f203b3a into swiftlang:main Jun 3, 2024
3 checks passed
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.

3 participants