Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Write link file list as a build command #6606

Merged
merged 1 commit into from May 25, 2023
Merged

Write link file list as a build command #6606

merged 1 commit into from May 25, 2023

Conversation

neonichu
Copy link
Member

@neonichu neonichu commented May 23, 2023 •
edited

This moves the generation of link file lists into the build system instead of doing it ad-hoc outside of the build. For this, we have a new WriteAuxiliaryFile tool and associated command that should be usable for any kind of writing of auxiliary files during the build.

Note that this change opted to not touch the existing infrastructure for in-process tools, so any inputs that are needed for the file generation will need to be flattened into a generic array of input nodes. The different types of file generation are keyed off a virtual node at the start of that array.

@neonichu neonichu requested a review from compnerd May 23, 2023 21:49
@neonichu neonichu self-assigned this May 23, 2023
@neonichu neonichu force-pushed the write-file-list-as-build-command branch from c72ae46 to d2fc83f Compare May 23, 2023 23:12
@neonichu
Copy link
Member Author

Updated some tests that were relying on the file existing after build planning to instead look at the build plan itself.

@neonichu
Copy link
Member Author

@swift-ci please smoke test

tomerd
tomerd reviewed May 23, 2023
View reviewed changes
Copy link
Member

@tomerd tomerd left a comment

Choose a reason for hiding this comment

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

this is great!

@neonichu
Copy link
Member Author

Failure is

Test Suite 'Selected tests' started at 2023-05-23 23:18:32.801Test Suite 'BuildPlanTests' started at 2023-05-23 23:18:32.803Test Case 'BuildPlanTests.testArchiving' started at 2023-05-23 23:18:32.803/home/build-user/swiftpm/Tests/BuildTests/BuildPlanTests.swift:4070: error: BuildPlanTests.testArchiving : XCTAssertTrue failed

this is one of the tests I updated and it does seem to have cases for different platforms, so makes sense that it is failing.

@neonichu neonichu force-pushed the write-file-list-as-build-command branch from d2fc83f to 734e28d Compare May 24, 2023 15:06
@neonichu
Copy link
Member Author

@swift-ci please smoke test

@neonichu neonichu force-pushed the write-file-list-as-build-command branch from 734e28d to ad99ee5 Compare May 24, 2023 17:55
@neonichu
Copy link
Member Author

There was a small typo in the test change.

@neonichu
Copy link
Member Author

@swift-ci please smoke test

@neonichu neonichu force-pushed the write-file-list-as-build-command branch from ad99ee5 to 9e01372 Compare May 24, 2023 19:01
@neonichu neonichu changed the title WIP: Write link file list as a build command Write link file list as a build command May 24, 2023
@neonichu neonichu marked this pull request as ready for review May 24, 2023 19:03
@neonichu
Copy link
Member Author

@swift-ci please smoke test

@neonichu
Copy link
Member Author

@swift-ci please smoke test windows

1 similar comment
@neonichu
Copy link
Member Author

@swift-ci please smoke test windows

@neonichu
Copy link
Member Author

Hm, seems like Windows job isn't starting again...

@neonichu
Copy link
Member Author

This should be ready for review now, but I do want to make sure we can also support implementing writeOutputFileMap() like this before landing this PR.

@neonichu
Copy link
Member Author

@swift-ci please smoke test windows

@neonichu
Copy link
Member Author

Windows failure is infra related.

@neonichu
Copy link
Member Author

Got output file maps working with the same infrastructure, so I think this should be good.

@neonichu neonichu force-pushed the write-file-list-as-build-command branch 2 times, most recently from b222551 to 88b203a Compare May 24, 2023 23:35
@neonichu
Copy link
Member Author

Oh interesting, looks like running swiftformat broke the code: Insufficient indentation of line in multi-line string literal.

@neonichu
Copy link
Member Author

neonichu commented May 25, 2023 •
edited

We actually can't format the code in testArchiving() anyway, it has to match the formatting of the LLBuild manifest we're generating. I reformatted the code now using Xcode since swiftformat would also break that assumption (in addition to anyway breaking the multiline string literal itself).

@neonichu neonichu force-pushed the write-file-list-as-build-command branch from 88b203a to c90da05 Compare May 25, 2023 00:02
@neonichu
Copy link
Member Author

@swift-ci please smoke test

@neonichu
Copy link
Member Author

@swift-ci please smoke test windows

@neonichu
Write link file list as a build command
9534ca1 
This moves the generation of link file lists into the build system instead of doing it ad-hoc outside of the build. For this, we have a new `WriteAuxiliaryFile` tool and associated command that should be usable for any kind of writing of auxiliary files during the build.

Note that this change opted to not touch the existing infrastructure for in-process tools, so any inputs that are needed for the file generation will need to be flattened into a generic array of input nodes. The different types of file generation are keyed off a virtual node at the start of that array.
@neonichu neonichu force-pushed the write-file-list-as-build-command branch from c90da05 to 9534ca1 Compare May 25, 2023 18:25
@neonichu
Copy link
Member Author

@swift-ci please smoke test

@neonichu
Copy link
Member Author

@swift-ci please smoke test windows

1 similar comment
@neonichu
Copy link
Member Author

@swift-ci please smoke test windows

tomerd
tomerd approved these changes May 25, 2023
View reviewed changes
Copy link
Member

@tomerd tomerd left a comment

Choose a reason for hiding this comment

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

nice work

@neonichu
Copy link
Member Author

@swift-ci please smoke test windows

@neonichu neonichu enabled auto-merge (squash) May 25, 2023 19:35
@neonichu
Copy link
Member Author

Windows failure is apple/swift#66138

@compnerd
Copy link
Collaborator

Please test with following PRs:
apple/swift#66138

@swift-ci please test windows platform

@neonichu neonichu merged commit ee7f064 into main May 25, 2023
5 checks passed
@neonichu neonichu deleted the write-file-list-as-build-command branch May 25, 2023 23:11
@ahoppen ahoppen mentioned this pull request Jul 12, 2023
Closed
@adam-fowler
Copy link

Would we be able to merge this into release/5.9. It fixes a major issue where sourcekit-lsp breaks build plugins

@neonichu
Copy link
Member Author

It has been in main for a while now, so we have a certain amount of confidence it won't cause major fallout. @tomerd wdyt about pulling this into 5.9?

@tomerd
Copy link
Member

tomerd commented Jul 13, 2023

sgtm

@neonichu
Copy link
Member Author

Hm, straight cherry-pick is not working, I am seeing a few test failures being introduced. So this won't be trivial to include in 5.9.

@neonichu
Copy link
Member Author

Opened a PR for the cherry-pick here but it needs investigation into the test issues that this causes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaxDesiatov MaxDesiatov MaxDesiatov left review comments

@tomerd tomerd tomerd approved these changes

@compnerd compnerd Awaiting requested review from compnerd

@abertelrud abertelrud Awaiting requested review from abertelrud

Assignees

@neonichu neonichu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@neonichu @compnerd @adam-fowler @tomerd @MaxDesiatov