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

Fix make build_release after changes to lib_internalSwiftSyntaxParser #493

Merged

Conversation

liamnichols
Copy link
Collaborator

Background

After switching to using StaticInternalSwiftSyntax parser on macOS, we become impacted by a bug when trying to build a fat binary:

Note: because of this bug if you want to depend on this target in SwiftPM and target multiple architectures in a single build, you must only depend on it from top level targets such as a test or executable target.

The problem comes when trying to use swift build --arch arm64 --arch x86_64 however I originally didn't think that we were doing this and forgot to check the Makefile. After seeing the issue reported in swiftlang/swift#490, I realised that we do need to work around this.

Changes

In this change, I follow the workaround that is used both in SwiftLint and Sourcery, which avoid the bug by building the arm64 and x86_64 binaries separately and then using otool to merge them together into one universal binary.

…ng library dependencies and duplicate symbols
Comment on lines 21 to +22
show_bin_path:
@echo ${BIN_PATH}
@echo ${EXECUTABLE}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's probably worth confirming the exact purpose of make show_bin_path now, since we have to manually create the universal binary, the output path (.build/periphery) is always known anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's used in the release script. Probably not quite as useful now, but still, it's nice to keep the logic in here.

@liamnichols liamnichols marked this pull request as ready for review May 2, 2022 17:14
@liamnichols
Copy link
Collaborator Author

The other thing I wanted to confirm... Was make build_release ever used on Linux? Or was it intended to be usable on Linux? iirc otool isn't available there so this approach isn't possible, but on Linux we don't need this anyway because the static lib_InternalSwiftSyntaxParser library is only used on macOS so it wouldn't have been a problem.

But then again, I didn't think universal binaries were a thing on Linux which leads me to think that make build_release was a macOS only thing anyway?

Copy link
Contributor

@ileitch ileitch left a comment

Choose a reason for hiding this comment

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

The other thing I wanted to confirm... Was make build_release ever used on Linux?

Nope, just macOS.

Comment on lines 21 to +22
show_bin_path:
@echo ${BIN_PATH}
@echo ${EXECUTABLE}
Copy link
Contributor

Choose a reason for hiding this comment

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

It's used in the release script. Probably not quite as useful now, but still, it's nice to keep the logic in here.

@ileitch
Copy link
Contributor

ileitch commented May 8, 2022

Thank you once again for all your hard work, @liamnichols. I really appreciate it 🙇

@ileitch ileitch merged commit 57b576f into peripheryapp:master May 8, 2022
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.

2 participants