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.

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

Symlink swift and swiftc to swift-driver, when available #69834

Merged
merged 4 commits into from
Jan 9, 2024

Conversation

artemcm
Copy link
Contributor

@artemcm artemcm commented Nov 14, 2023

  • Remove redundant calls to add_swift_tool_symlink on /bin executables of the driver CMake target
  • Symlink swift and swiftc to swift-driver, when available (when earlyswiftdriver was built with the host toolchain (default))
  • Install host-built swift-driver and swift-help when installing the 'compiler' component
  • Add symlinks for the legacy driver invocation (for emergency fallback invoked from swift-driver)

@artemcm
Copy link
Contributor Author

artemcm commented Nov 14, 2023

@swift-ci test

@artemcm
Copy link
Contributor Author

artemcm commented Nov 14, 2023

@artemcm artemcm changed the title Symlink 'swift' and 'swiftc' to 'swift-driver', when available Symlink swift and swiftc to swift-driver, when available Nov 14, 2023
@artemcm artemcm marked this pull request as ready for review November 14, 2023 23:44
@@ -157,15 +182,6 @@ swift_create_post_build_symlink(swift-frontend
DESTINATION "swift-parse-test${CMAKE_EXECUTABLE_SUFFIX}"
WORKING_DIRECTORY "${SWIFT_RUNTIME_OUTPUT_INTDIR}")

add_swift_tool_symlink(swift swift-frontend compiler)
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can tell what happened here is that #6053 added this and removed the others, but that was somehow lost in the conflict resolution in #10337. I haven't looked at the LLVM closely, but I assume we should probably prefer add_swift_tool_symlink over swift_create_post_build_symlink and swift_install_in_component? Any thoughts @edymtt/@compnerd?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One thing about add_swift_tool_symlink is that it requires the "source" be a component. So we can't simply call:

add_swift_tool_symlink(swift swift-driver compiler)

Because that leads to:

CMake Error at /Volumes/Data/GHWorkspace/build/Ninja-Release/llvm-macosx-arm64/lib/cmake/llvm/AddLLVM.cmake:2168 (get_target_property):
  get_target_property() called with non-existent target "swift-driver".

Copy link
Member

Choose a reason for hiding this comment

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

@bnbarham yes, we do prefer the add_swift_tool_symlink because it correctly handles the install of the link (or in the case of Windows, copy 😠). This makes it significantly better for building the toolchain distribution. Without that we will need to add a whole slew of custom install logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear, we have that logic already (we have both today):

swift_install_in_component(FILES "${SWIFT_RUNTIME_OUTPUT_INTDIR}/swift${CMAKE_EXECUTABLE_SUFFIX}"
                           DESTINATION "bin"
                           COMPONENT compiler)

One thing about add_swift_tool_symlink is that it requires the "source" be a component. So we can't simply call:

We could fix that by adding a custom target for the early swift driver (which does the copy) right? ie. change swift_create_early_driver_copies to instead use add_custom_command + add_custom_target rather than configure_file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already have a slew of custom install logic though, with swift_install_in_component later in this file.

We can pursue a refactor here to replace both swift_create_post_build_symlink and swift_install_in_component with LLVM tooling, but as-is, the logic is already there, and add_swift_tool_symlink would not work for this use-case for the reason described above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, swift_install_in_component does, I believe, already do Windows-specific things (copy instead of symlink) so I don't think this will functionally change much here.

@artemcm
Copy link
Contributor Author

artemcm commented Nov 17, 2023

@artemcm artemcm force-pushed the DefaultNewDriverSymlink branch from e613b4b to f380790 Compare November 17, 2023 20:08
@artemcm
Copy link
Contributor Author

artemcm commented Nov 17, 2023

1 similar comment
@artemcm
Copy link
Contributor Author

artemcm commented Nov 27, 2023

@artemcm artemcm force-pushed the DefaultNewDriverSymlink branch 2 times, most recently from 16b9012 to b89b5d7 Compare November 30, 2023 17:45
@artemcm
Copy link
Contributor Author

artemcm commented Nov 30, 2023

@artemcm
Copy link
Contributor Author

artemcm commented Nov 30, 2023

swiftlang/swift-driver#1485
@swift-ci build toolchain

@artemcm artemcm force-pushed the DefaultNewDriverSymlink branch from b89b5d7 to f935713 Compare December 4, 2023 18:56
@artemcm
Copy link
Contributor Author

artemcm commented Dec 6, 2023

@compnerd
Copy link
Member

compnerd commented Dec 8, 2023

@swift-ci please build toolchain Windows platform

@artemcm
Copy link
Contributor Author

artemcm commented Dec 8, 2023

swiftlang/swift-driver#1485
@swift-ci test macOS platform

3 similar comments
@artemcm
Copy link
Contributor Author

artemcm commented Dec 13, 2023

swiftlang/swift-driver#1485
@swift-ci test macOS platform

@artemcm
Copy link
Contributor Author

artemcm commented Dec 19, 2023

swiftlang/swift-driver#1485
@swift-ci test macOS platform

@artemcm
Copy link
Contributor Author

artemcm commented Jan 2, 2024

swiftlang/swift-driver#1485
@swift-ci test macOS platform

@artemcm artemcm force-pushed the DefaultNewDriverSymlink branch from f935713 to 6a181fb Compare January 3, 2024 23:45
@artemcm
Copy link
Contributor Author

artemcm commented Jan 4, 2024

@artemcm
Copy link
Contributor Author

artemcm commented Jan 8, 2024

@swift-ci please build Windows toolchain

@artemcm
Copy link
Contributor Author

artemcm commented Jan 8, 2024

@swift-ci please build toolchain Windows

1 similar comment
@artemcm
Copy link
Contributor Author

artemcm commented Jan 9, 2024

@swift-ci please build toolchain Windows

@artemcm artemcm merged commit 060d822 into swiftlang:main Jan 9, 2024
6 checks passed
@artemcm artemcm deleted the DefaultNewDriverSymlink branch January 9, 2024 20:50
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