Skip to content

bootstrap: allow building with shared BlocksRuntime #1631

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 1 commit into from
Jul 20, 2018

Conversation

compnerd
Copy link
Member

No description provided.

@@ -579,6 +579,8 @@ class llbuild(object):
if self.args.foundation_path:
link_command.extend(["-L", self.args.foundation_path])
if self.args.libdispatch_build_dir:
link_command.extend(['-L', self.args.libdispatch_build_dir,
'-Xlinker', '-lBlocksRuntime'])
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be
link_command.extend(['-L', self.args.libdispatch_build_dir, '-lBlocksRuntime'])

Copy link
Member Author

Choose a reason for hiding this comment

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

It seemed that the actual linked library names are passed directly through the driver in the rest of the build. Im just following the pattern. I believe that the driver will forward the options either way.

@compnerd
Copy link
Member Author

@swift-ci please test

@compnerd
Copy link
Member Author

The failure is weird, llbuild linkage is failing, which should not be impacted by addition only linkage for BlocksRuntime.

@compnerd
Copy link
Member Author

@swift-ci please test Linux platform

@compnerd
Copy link
Member Author

@aciidb0mb3r - okay to merge this?

@compnerd
Copy link
Member Author

@swift-ci please smoke test

@@ -1125,6 +1127,9 @@ def main():
symlink_force(os.path.join(args.foundation_path, 'libFoundation.so'),
libswiftdir)
if args.libdispatch_build_dir:
symlink_force(os.path.join(args.libdispatch_build_dir,
'libBlocksRuntime.so'),
libswiftdir)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this symlink?

Copy link
Member Author

Choose a reason for hiding this comment

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

Without that symlink, the tests tended to fail. libdispatch would not be found, but by keeping the symlink there, which is one of the paths that linker/loader consult, the tests are happier.

@aciidgh
Copy link
Contributor

aciidgh commented Jul 19, 2018

Can you explain what is this and why do clients need to link to it manually? Also, is this a hard dependency?

@compnerd
Copy link
Member Author

libBlocksRuntime is the runtime for blocks. The closures in swift are actually blocks, and need a runtime for to support them. Currently, it was built static and put into libdispatch and a couple of other entities. However, there are globals which need to be uniqued across the image, and that requires that we have the BlocksRuntime be shared. This is setup to convert the build of the Blocks Runtime to a shared library.

@aciidgh
Copy link
Contributor

aciidgh commented Jul 20, 2018

Thanks for the explanation, @compnerd! It sounds like this is a dependency of the libdispatch or the Swift compiler itself. I don't think SwiftPM (or every Swift based Swift.org project) should need to explicitly configure search paths for this dependency.

@compnerd
Copy link
Member Author

@aciidb0mb3r, unfortunately, during the complex CI build, that isnt really feasible. When the components are installed in the proper location (/usr/lib), they will be found without a search path. Switching over to the shared build will allow us to move the library into swift itself, and the search path can be removed at that point. I think it would be more reasonable to try to move this into the driver at that point. This change is basically staging work.

@aciidgh
Copy link
Contributor

aciidgh commented Jul 20, 2018

@compnerd Ah, okay. That sounds reasonable to me then! Feel free to merge.

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