-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Swift: teach autobuilder about SPM, CocoaPods, and Carthage #13979
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
Conversation
3d88216
to
456a010
Compare
91851eb
to
328609d
Compare
fa409b1
to
6a5e539
Compare
Tests for SPM are not here, I'll add them in a later PR when I convert Xcode autobuilder into Swift autobuilder and make it also available on Linux. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
auto pod = std::string(getenv("CODEQL_SWIFT_POD_EXEC") ?: ""); | ||
auto carthage = std::string(getenv("CODEQL_SWIFT_CARTHAGE_EXEC") ?: ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL about this ?:
GNU extension that I knew nothing about 🙂
As a nit, this can be made more standard conforming using std::string_view
, which contrary to std::string
accepts a nullptr
(and treats it equivalent to ""
). Or move the getenv
as a nested if
, for example
if (!target.podfiles.empty()) {
if (auto pod = getenv("CODEQL_SWIFT_POD_EXEC")) {
pod_install(pod, podfile, dryRun);
}
}
or as one single if with initialization:
if (auto pod = getenv("CODEQL_SWIFT_POD_EXEC"); pod && !target.podfiles.empty()) {
pod_install(pod, podfile, dryRun);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first snippet is not exactly correct as we may have podfiles on linux, but won't have pod
executable there.
And I don't find the second necessarily better than what we have now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as far as I understand, the first snippet is equivalent, as the pod installation will happen only if both the conditions are met. The conditions are just evaluated in swapped order (and the env variable won't be inspected if podfiles
is empty).
What itches me here, is the use of a non-standard extension, that might also be surprising to some developers (it was a bit to me). On the other hand, we do enforce compilation with clang, so maybe the non-standardness is not such a big deal... It's just that I personally only use non-standard C++ stuff when there is no sensible alternative, which I think there is here.
But that's a nit, I'm also kinda ok with leaving this as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is as non-standard as #pragma once
😄
@@ -217,41 +233,29 @@ static std::vector<fs::path> collectFiles(const std::string& workingDir) { | |||
continue; | |||
} | |||
if (p.extension() == ".xcodeproj" || p.extension() == ".xcworkspace") { | |||
files.push_back(p); | |||
structure.xcodeFiles.push_back(p); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe as you are anyway creating this structure with fields for the different kinds of files, you can also split .xcodeproj
and .xcworkspace
in separate xcodeProjects
and xcodeWorkspaces
fields of this structure, and avoid testing the extension again later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I thought about this but decided not to as these two go hand in hand and it doesn't make much difference if we do it the other way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be a tad cleaner, but I'll leave it to your judgement 🙂
Co-authored-by: Paolo Tranquilli <redsun82@github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only nits remaining, I'll be happy to reapprove if you do decide to change things around.
No description provided.