Skip to content

[build-script] Add the installation prefix to the toolchain path #30565

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

Merged
merged 1 commit into from
Apr 20, 2020
Merged

[build-script] Add the installation prefix to the toolchain path #30565

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions utils/swift_build_support/swift_build_support/products/pythonkit.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ def should_build(self, host_target):
def build(self, host_target):
toolchain_path = targets.toolchain_path(self.args.install_destdir,
self.args.install_prefix)
swiftc = os.path.join(toolchain_path, 'usr', 'bin', 'swiftc')
swiftc = os.path.join(toolchain_path, 'bin', 'swiftc')

# FIXME: this is a workaround for CMake <3.16 which does not correctly
# generate the build rules if you are not in the build directory. As a
Expand All @@ -52,7 +52,7 @@ def build(self, host_target):
self.toolchain.cmake,
'-G', 'Ninja',
'-D', 'BUILD_SHARED_LIBS=YES',
'-D', 'CMAKE_INSTALL_PREFIX={}/usr'.format(
'-D', 'CMAKE_INSTALL_PREFIX={}'.format(
self.install_toolchain_path()),
'-D', 'CMAKE_MAKE_PROGRAM={}'.format(self.toolchain.ninja),
'-D', 'CMAKE_Swift_COMPILER={}'.format(swiftc),
Expand Down
2 changes: 1 addition & 1 deletion utils/swift_build_support/swift_build_support/products/swiftpm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ def run_bootstrap_script(self, action, host_target, additional_params=[]):
script_path = os.path.join(
self.source_dir, 'Utilities', 'bootstrap')
toolchain_path = self.install_toolchain_path()
swiftc = os.path.join(toolchain_path, "usr", "bin", "swiftc")
swiftc = os.path.join(toolchain_path, "bin", "swiftc")

# FIXME: We require llbuild build directory in order to build. Is
# there a better way to get this?
Expand Down
4 changes: 2 additions & 2 deletions utils/swift_build_support/swift_build_support/products/tensorflow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ def should_build(self, host_target):
def build(self, host_target):
toolchain_path = targets.toolchain_path(self.args.install_destdir,
self.args.install_prefix)
swiftc = os.path.join(toolchain_path, 'usr', 'bin', 'swiftc')
swiftc = os.path.join(toolchain_path, 'bin', 'swiftc')

# FIXME: this is a workaround for CMake <3.16 which does not correctly
# generate the build rules if you are not in the build directory. As a
Expand All @@ -52,7 +52,7 @@ def build(self, host_target):
self.toolchain.cmake,
'-G', 'Ninja',
'-D', 'BUILD_SHARED_LIBS=YES',
'-D', 'CMAKE_INSTALL_PREFIX={}/usr'.format(
'-D', 'CMAKE_INSTALL_PREFIX={}'.format(
self.install_toolchain_path()),
'-D', 'CMAKE_MAKE_PROGRAM={}'.format(self.toolchain.ninja),
'-D', 'CMAKE_Swift_COMPILER={}'.format(swiftc),
Expand Down
4 changes: 3 additions & 1 deletion utils/swift_build_support/swift_build_support/targets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,5 +284,7 @@ def toolchain_path(install_destdir, install_prefix):
built_toolchain_path = install_destdir
if platform.system() == 'Darwin':
# The prefix is an absolute path, so concatenate without os.path.
built_toolchain_path += darwin_toolchain_prefix(install_prefix)
built_toolchain_path += darwin_toolchain_prefix(install_prefix) + "/usr"
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 if calling this a "toolchain path" is good terminology, since on Darwin that would mean the .xctoolchain directory. That's also what the documentation string for this method says. After this change, it's always an absolute form of the install_prefix. I'm not sure what to call that 🤔

By the way, darwin_toolchain_prefix removes usr from install_prefix, and then this adds it back.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if calling this a "toolchain path" is good terminology

I have no idea why this is needed on Darwin or what the common paths are there, I just tried to keep the behavior the same.

By the way, darwin_toolchain_prefix removes usr from install_prefix, and then this adds it back.

Yes, as I noted in my previous comment, that is what was being done everywhere already, so rather than adding it back in a dozen different places, I simply added it back once. 😄

I figured it might be because install_prefix on Darwin doesn't necessarily end with /usr, so darwin_toolchain_prefix() just removes that last component and the rest of the code replaces it with /usr. But that's just a guess, given my professed ignorance of the Darwin toolchain.

Just let me know what should be done, as this pull actually starts using install_prefix on non-Darwin platforms, rather than assuming it's always /usr.

else:
built_toolchain_path += install_prefix
return built_toolchain_path