Prefer `add_swift_tool_symlink` to install symlink for `swift-frontend` #61668

edymtt · 2022-10-21T16:27:54Z

Also adapt to new signature for llvm_install_symlink and match the usage pattern employed by other LLVM projects.

Addresses rdar://101396797

edymtt · 2022-10-21T16:28:09Z

@swift-ci please smoke test

cmake/modules/AddSwift.cmake

compnerd · 2022-10-21T16:30:54Z

I think that more than just the smoke test, we should be building full toolchains for this. I'm concerned that this might impact the file system layout and thus packaging (at least for Windows).

edymtt · 2022-10-21T16:33:41Z

@swift-ci please build toolchain

and match the usage pattern employed by other LLVM projects. For context about the underlying change see https://reviews.llvm.org/D117977 Addresses rdar://101396797

edymtt · 2022-10-24T16:35:06Z

@swift-ci please smoke test

edymtt · 2022-10-24T16:35:16Z

@swift-ci please build toolchain

compnerd

Thanks! This seems pretty close. I am curious about a possible drift between the actual binaries being installed and the symlinks though.

compnerd · 2022-10-24T16:39:01Z

CMakeLists.txt

  add_definitions("-fprofile-instr-use=${SWIFT_PROFDATA_FILE}")
 endif()

+set(SWIFT_TOOLS_INSTALL_DIR "bin" CACHE PATH


Should we ensure that this matches the install bin dir?

I tried to search for such a parameter I could reuse, but I was not able to find it -- it seems we hardcode bin and/or we rely on preexisting defaults (like it was the case for llvm_install_symlink pre LLVM 15)

Updating all the places to use a proper parameter would require more work and qualification outside of the scope of unblocking next -- so in this context I leaned toward adding the variable to the main CMakeLists.txt to "highlight" this concern in future reviews.

After a bit more investigation, wondering if I should instead solve this issue inside of tools/driver/CMakeLists.txt -- in particular noticing that in there add_swift_tool_symlink and swift_install_in_component are laying down the same symlinks during installation (albeit with different means), so there could be a chance to simplify this part instead of adding to it

https://github.com/apple/swift/blob/dc8d37d6b5adeb2371755844445ef5b3021c789a/tools/driver/CMakeLists.txt#L114

https://github.com/apple/swift/blob/dc8d37d6b5adeb2371755844445ef5b3021c789a/tools/driver/CMakeLists.txt#L123-L125

#6053 seems to suggest that may have been the original intent for adding add_swift_tool_symlink in the first place

Proposing one such approach in f94b96e -- preferring add_swift_tool_symlink to create the symlink during installation, and augmenting it with a destination argument

Depending on how we want to do it, we can base this on ${LLVM_TOOLS_INSTALL_DIR} or ${CMAKE_INSTALL_BINDIR}.

Now I see -- I thought the question was about the risk of installing the symlinks in a different directory than the target program, but instead it seems we were more concerned on using a sane default value for the setting.

I reverted to the previous approach and used ${CMAKE_INSTALL_BINDIR} as the default value in e7c4b19

Also ensure we don't symlink to swift-frontend twice

edymtt · 2022-10-25T15:34:12Z

@swift-ci please build toolchain

edymtt · 2022-10-25T15:34:24Z

@swift-ci please smoke-test

gottesmm · 2022-10-25T20:52:36Z

cmake/modules/AddSwift.cmake

+  set(SWIFT_TOOLS_INSTALL_DIR destination)
  llvm_add_tool_symlink(SWIFT ${name} ${dest} ALWAYS_GENERATE)
  llvm_install_symlink(SWIFT ${name} ${dest} ALWAYS_GENERATE COMPONENT ${component})
+  unset(SWIFT_TOOLS_INSTALL_DIR)


Instead of creating an LLVM component called "compiler", can you instead use the default behavior of LLVM (which should create install-${name} as an install command) and instead use something like the following to register that llvm component (swift-frontend) with the swift component (compiler).

function (add_dependency_from_llvm_component_to_swift_component swift-target-name swift-component) add_dependencies(install-${swift-target-name} install-${swift-component}) endfunction()

From my experiments, it seems I cannot drop the COMPONENT argument -- if I do, the cmake_install.cmake script will not install the symlink when specifying our Swift components (e.g. -DCMAKE_INSTALL_COMPONENT=compiler), but when specifying the name of the target program (if ALWAYS_GENERATE is provided) or the source one (otherwise), which is not something that we currently allow in the Swift build system.

As discussed offline, I've attempted to lean a bit more on the LLVM functions around components by (a) dropping ALWAYS_GENERATE and (b) configuring an LLVM component for swift-frontend to avoid failures -- you can find this in 87f8906 (although I have mixed feelings about it)

Use CMAKE_INSTALL_BINDIR as initial value for SWIFT_TOOLS_INSTALL_DIR

edymtt · 2022-10-26T22:30:06Z

@swift-ci please build toolchain

edymtt · 2022-10-27T20:19:58Z

I've noticed that this PR is now addressing two different concerns:

adapting to the new signature of add_swift_tool_symlink
avoid creating symlinks for swift-frontend twice, and possibly take the chance to lean a bit more into the LLVM component model

Those have different implications and urgency, so I thought to split this PR in two:

Adapt to new signature for llvm_install_symlink... #61769
- this would be needed to unblock the ongoing rebranch
Create symlinks for swift-frontend only once #61768
- this does not address any immediate issue, and this may likely warrant more discussion

edymtt requested review from compnerd, drexin, etcwilde and gottesmm October 21, 2022 16:27

compnerd requested changes Oct 21, 2022

View reviewed changes

cmake/modules/AddSwift.cmake Outdated Show resolved Hide resolved

Adapt to new signature for llvm_install_symlink...

79b5a1d

and match the usage pattern employed by other LLVM projects. For context about the underlying change see https://reviews.llvm.org/D117977 Addresses rdar://101396797

edymtt force-pushed the next-adapt-to-new-llvm_install_symlink-signature branch from c268511 to 79b5a1d Compare October 24, 2022 16:30

edymtt changed the title ~~[next] adapt to new signature for llvm_install_symlink~~ Adapt to new signature for llvm_install_symlink Oct 24, 2022

edymtt changed the title ~~Adapt to new signature for llvm_install_symlink~~ Adapt to new signature for llvm_install_symlink... Oct 24, 2022

edymtt requested a review from compnerd October 24, 2022 16:35

compnerd reviewed Oct 24, 2022

View reviewed changes

Augment add_swift_tool_symlink to set the target destination

f94b96e

Also ensure we don't symlink to swift-frontend twice

edymtt changed the title ~~Adapt to new signature for llvm_install_symlink...~~ Prefer add_swift_tool_symlink to install symlink for swift-frontend Oct 25, 2022

edymtt mentioned this pull request Oct 25, 2022

Attempt to generate a next toolchain with #61668 #61721

Closed

Reference parameter correctly in macro

d86fb5b

gottesmm reviewed Oct 25, 2022

View reviewed changes

edymtt added 2 commits October 26, 2022 12:01

Revert to previous approach and take review feedback

e7c4b19

Use CMAKE_INSTALL_BINDIR as initial value for SWIFT_TOOLS_INSTALL_DIR

Try to adhere to the LLVM component system when generating the symlinks

87f8906

edymtt closed this Oct 31, 2022

edymtt deleted the next-adapt-to-new-llvm_install_symlink-signature branch October 31, 2022 14:16

Prefer add_swift_tool_symlink to install symlink for swift-frontend #61668

Prefer add_swift_tool_symlink to install symlink for swift-frontend #61668

Uh oh!

Conversation

edymtt commented Oct 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edymtt commented Oct 21, 2022

Uh oh!

Uh oh!

compnerd commented Oct 21, 2022

Uh oh!

edymtt commented Oct 21, 2022

Uh oh!

edymtt commented Oct 24, 2022

Uh oh!

edymtt commented Oct 24, 2022

Uh oh!

compnerd left a comment

Choose a reason for hiding this comment

Uh oh!

compnerd Oct 24, 2022

Choose a reason for hiding this comment

Uh oh!

edymtt Oct 24, 2022

Choose a reason for hiding this comment

Uh oh!

edymtt Oct 24, 2022

Choose a reason for hiding this comment

Uh oh!

edymtt Oct 25, 2022

Choose a reason for hiding this comment

Uh oh!

etcwilde Oct 25, 2022

Choose a reason for hiding this comment

Uh oh!

edymtt Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edymtt commented Oct 25, 2022

Uh oh!

edymtt commented Oct 25, 2022

Uh oh!

gottesmm Oct 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edymtt Oct 26, 2022

Choose a reason for hiding this comment

Uh oh!

edymtt commented Oct 26, 2022

Uh oh!

edymtt commented Oct 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Prefer `add_swift_tool_symlink` to install symlink for `swift-frontend` #61668

Prefer `add_swift_tool_symlink` to install symlink for `swift-frontend` #61668

edymtt commented Oct 21, 2022 •

edited

Loading

edymtt Oct 26, 2022 •

edited

Loading

gottesmm Oct 25, 2022 •

edited

Loading