Skip to content

[clang] Add deployment target env vars to features.json #4803

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
Jul 8, 2022
Merged

[clang] Add deployment target env vars to features.json #4803

merged 1 commit into from
Jul 8, 2022

Conversation

jansvoboda11
Copy link

@jansvoboda11 jansvoboda11 commented Jun 6, 2022 •
edited
Loading

This adds new entry to the features.json file with list of environment variables used when deciding the deployment target. To be able to make these values depend on #ifdef directives, we run the file through the preprocessor instead of simple copy.

rdar://91377604

@jansvoboda11 jansvoboda11 mentioned this pull request Jun 6, 2022
Merged
@jansvoboda11 jansvoboda11 requested review from bnbarham and hyp June 6, 2022 12:55
@bnbarham
Copy link

bnbarham commented Jun 6, 2022

Would you mind cherry-picking this to stable/20220421 in addition to any other cherry-pick you do, as well as cherry-picking the corresponding swift change to rebranch?

Ideally merge the swift change a little before this one so that we don't have any erroneous failures.

@jansvoboda11 jansvoboda11 force-pushed the jan_svoboda/features_json branch from de4e222 to 5e59bb1 Compare June 7, 2022 17:21
@jansvoboda11 jansvoboda11 changed the base branch from next to stable/20211026 June 7, 2022 17:21
compnerd
compnerd requested changes Jun 7, 2022
View reviewed changes
@jansvoboda11 jansvoboda11 force-pushed the jan_svoboda/features_json branch from 5cf312a to d5fb71c Compare June 9, 2022 07:24
@jansvoboda11 jansvoboda11 changed the title [clang] Add deploment target env vars to features.json [clang] Add deployment target env vars to features.json Jun 9, 2022
compnerd
compnerd reviewed Jun 9, 2022
View reviewed changes
clang/tools/driver/CMakeLists.txt Outdated
${features_file_src}
${features_file_dest}
DEPENDS ${features_file_src})
configure_file(${features_file_src} ${features_file_dest})
Copy link
Member

Choose a reason for hiding this comment

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

To be safer for the substitutions, I tend to use the @ONLY option so that we can mark the substitutions as @NAME@ where NAME is a define that we control and specify the value using add_definitions or set_soureces_properties.

Copy link
Author

Choose a reason for hiding this comment

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

I can put @ONLY in this PR.

We only need a variable in a downstream repo, so I'll try to make it work there.

Can you show me an example of configure_file using add_definitions and set_source_files_properties? The following doesn't work (using @FOO in the source file):

set(FOO "x")
configure_file(${features_file_src} ${features_file_dest} @ONLY)
set_source_files_properties(${features_file_src} PROPERTIES BAR "${FOO}")

Does this mechanism hide variables not explicitly set through set_source_files_properties or add_definitions?

@jansvoboda11
Copy link
Author

@swift-ci please test

@jansvoboda11 jansvoboda11 mentioned this pull request Jun 21, 2022
Closed
@jansvoboda11 jansvoboda11 force-pushed the jan_svoboda/features_json branch from d5fb71c to d392845 Compare June 22, 2022 17:44
@jansvoboda11
Copy link
Author

@swift-ci please test

@jansvoboda11 jansvoboda11 force-pushed the jan_svoboda/features_json branch from d392845 to 0872120 Compare June 23, 2022 07:11
@jansvoboda11
Copy link
Author

@swift-ci please test

@jansvoboda11
Copy link
Author

Error seems unrelated, but I'm unable to merge 🤷

error: terminated(1): /Users/ec2-user/jenkins/workspace/apple-llvm-project-pr-macos/branch-stable/20211026/build/buildbot_incremental/swiftpm-macosx-x86_64/x86_64-apple-macosx/release/swiftpm-xctest-helper /Users/ec2-user/jenkins/workspace/apple-llvm-project-pr-macos/branch-stable/20211026/build/buildbot_incremental/swiftpm-macosx-x86_64/x86_64-apple-macosx/release/SwiftPMPackageTests.xctest /var/folders/pf/p5g2vb2j0yx1x6vlmxdppc700000gn/T/TemporaryFile.H3H85G output:
    error: unableToLoadBundle("/Users/ec2-user/jenkins/workspace/apple-llvm-project-pr-macos/branch-stable/20211026/build/buildbot_incremental/swiftpm-macosx-x86_64/x86_64-apple-macosx/release/SwiftPMPackageTests.xctest")
    2022-06-23 09:56:36.561 swiftpm-xctest-helper[90844:7422687] Error loading /Users/ec2-user/jenkins/workspace/apple-llvm-project-pr-macos/branch-stable/20211026/build/buildbot_incremental/swiftpm-macosx-x86_64/x86_64-apple-macosx/release/SwiftPMPackageTests.xctest/Contents/MacOS/SwiftPMPackageTests:  dlopen(/Users/ec2-user/jenkins/workspace/apple-llvm-project-pr-macos/branch-stable/20211026/build/buildbot_incremental/swiftpm-macosx-x86_64/x86_64-apple-macosx/release/SwiftPMPackageTests.xctest/Contents/MacOS/SwiftPMPackageTests, 265): Library not loaded: @rpath/libswift_Concurrency.dylib
      Referenced from: /Users/ec2-user/jenkins/workspace/apple-llvm-project-pr-macos/branch-stable/20211026/build/buildbot_incremental/swiftpm-macosx-x86_64/x86_64-apple-macosx/release/SwiftPMPackageTests.xctest/Contents/MacOS/SwiftPMPackageTests
      Reason: image not found

@bnbarham
Copy link

I believe that's fixed now

@bnbarham
Copy link

@swift-ci please test macOS platform

@jansvoboda11 jansvoboda11 merged commit 7894920 into stable/20211026 Jul 8, 2022
@jansvoboda11 jansvoboda11 deleted the jan_svoboda/features_json branch July 8, 2022 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compnerd compnerd compnerd requested changes

@jakepetroules jakepetroules jakepetroules approved these changes

@bnbarham bnbarham bnbarham approved these changes

@hyp hyp Awaiting requested review from hyp

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jansvoboda11 @bnbarham @compnerd @jakepetroules