Skip to content

Add '--clang' flag to runner.py #586

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

Jump to bottom
Merged
merged 1 commit into from
Oct 7, 2021
Merged

Add '--clang' flag to runner.py #586

merged 1 commit into from
Oct 7, 2021

Conversation

rapidsna
Copy link
Contributor

@rapidsna rapidsna commented Sep 13, 2021 •
edited
Loading

Pull Request Description

'--clang' provides a way to build projects with a custom clang binary.
With '--clang-source-path', the 'run' script builds clang binary from
the clang source in the path and provides this binary path to 'runner.py'
using the '--clang' flag.

Acceptance Criteria

To be accepted into the Swift source compatibility test suite, a project must:

  • be an Xcode or swift package manager project
  • support building on either Linux or macOS
  • target Linux, macOS, or iOS/tvOS/watchOS device
  • be contained in a publicly accessible git repository
  • maintain a project branch that builds against Swift 4.0 and passes any unit tests
  • have maintainers who will commit to resolve issues in a timely manner
  • be compatible with the latest GM/Beta versions of Xcode and swiftpm
  • add value not already included in the suite
  • be licensed with one of the following permissive licenses:
    • BSD
    • MIT
    • Apache License, version 2.0
    • Eclipse Public License
    • Mozilla Public License (MPL) 1.1
    • MPL 2.0
    • CDDL
  • pass ./project_precommit_check script run

Ensure project meets all listed requirements before submitting a pull request.

@rapidsna rapidsna requested a review from shahmishal September 13, 2021 19:22
@rapidsna rapidsna force-pushed the add-clangc-public branch 3 times, most recently from e49379a to 426f901 Compare September 13, 2021 21:25
@rapidsna
Copy link
Contributor Author

@swift-ci test

@rapidsna rapidsna changed the title Add clangc public Add clangc flag to runner.py Sep 13, 2021
@rapidsna
Copy link
Contributor Author

@swift-ci test

ahoppen
ahoppen reviewed Sep 24, 2021
View reviewed changes
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

The whole approach of swapping a clang executable into the toolchain seems awfully fragile to me (see the amount of comments I left in that file). Is there a reason why you can’t just specify the clang executable by setting the CC environment variable similar to how we set SWIFT_EXEC?

I also added some comments regarding code style inline.

@ahoppen ahoppen self-assigned this Sep 24, 2021
@rapidsna
Copy link
Contributor Author

Thanks @ahoppen for reviewing my PR!

The whole approach of swapping a clang executable into the toolchain seems awfully fragile to me (see the amount of comments I left in that file). Is there a reason why you can’t just specify the clang executable by setting the CC environment variable similar to how we set SWIFT_EXEC?

The reason why we couldn't just specify a clang executable path is because that doesn't work with Xcode project build. xcodebuild always finds the clang binary from a toolchain location so we need to have a whole toolchain or swap the clang executables in the toolchain path. Some users may want to test the build with their custom clang and I don't expect users should generate a whole xcode toolchain so being able to specify their custom clang executable will be very useful.

I also added some comments regarding code style inline.

Thanks!

@rapidsna
Copy link
Contributor Author

rapidsna commented Oct 4, 2021

@swift-ci test

@rapidsna
Copy link
Contributor Author

rapidsna commented Oct 4, 2021

@swift-ci test

@rapidsna
Copy link
Contributor Author

rapidsna commented Oct 4, 2021

Thanks @ahoppen I addressed the style comments, and as you pointed out it now adds CC=clangc to Xcode flags instead of binary replacement. I removed the code that does it.

ahoppen
ahoppen reviewed Oct 5, 2021
View reviewed changes
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

I think this is much cleaner with the CC environment variable now. Thanks!

I’ve added some more comments inline.

run Outdated
'-DLLVM_TARGETS_TO_BUILD=X86;AArch64;ARM',
source_path]
common.check_execute(cmake_command)
common.check_execute(['xcrun', 'ninja'])
Copy link
Member

Choose a reason for hiding this comment

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

I suppose you could just use ninja_path here again, right? There’s no need to look it up again through xcrun. Would also make it more robust IMHO if the user xcode-selected a different version of Xcode between the two runs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

common.check_execute(['xcrun', 'ninja']) is to build the project with ninja. I'll add some comments.

run Outdated
'-DCMAKE_MAKE_PROGRAM={}'.format(ninja_path),
'-DLLVM_ENABLE_PROJECTS=clang;llvm',
'-DCMAKE_BUILD_TYPE=Release',
'-DCLANG_APPLE_BUILD_VERSION_STRING=13000000',
Copy link
Member

Choose a reason for hiding this comment

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

I suppose you are hardcoding this version number because it’s the current clang version and it’s bound to get out of date at some point. I’d prefer something completely non-sensical like 99999999 or to remove it, if possible. Otherwise we’ll wonder in a couple of years why this needs to be 13000000.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That specific string is necessary for building an internal project, but I removed it from this PR.

ahoppen
ahoppen approved these changes Oct 6, 2021
View reviewed changes
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Thanks for being patient and addressing all my comments. Looks good to me now 👍

@ahoppen
Copy link
Member

ahoppen commented Oct 6, 2021

@swift-ci Please test

@rapidsna rapidsna changed the title Add clangc flag to runner.py Add '--clang' flag to runner.py Oct 6, 2021
@rapidsna
Add '--clang' flag to runner.py
ce448af 
'--clang' provides a way to build Xcode projects with a custom clang binary,
by adding 'CC=clangc' flag to xcode.

With '--clang-source-path', the 'run' script builds clang binary from
the clang source in the path and provides this binary path to 'runner.py'
using the '--clang' flag.
@rapidsna
Copy link
Contributor Author

rapidsna commented Oct 6, 2021

@swift-ci test

@rapidsna
Copy link
Contributor Author

rapidsna commented Oct 6, 2021

@shahmishal gentle reminder

shahmishal
shahmishal approved these changes Oct 6, 2021
View reviewed changes
Copy link
Member

@shahmishal shahmishal left a comment

Choose a reason for hiding this comment

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

LGTM! thanks

@rapidsna
Copy link
Contributor Author

rapidsna commented Oct 7, 2021

@swift-ci test

@rapidsna
Copy link
Contributor Author

rapidsna commented Oct 7, 2021

@shahmishal The test failures are on Swift Packages and don't seem to be related to this change. Would you mind merging it on my behalf?

@shahmishal shahmishal merged commit b8f9309 into main Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shahmishal shahmishal shahmishal approved these changes

@ahoppen ahoppen ahoppen approved these changes

@clackary clackary Awaiting requested review from clackary

Assignees

@ahoppen ahoppen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rapidsna @ahoppen @shahmishal