Skip to content
This repository has been archived by the owner on Jan 18, 2022. It is now read-only.

Fix xcode paths #1040

Merged
merged 7 commits into from
Jul 17, 2019
Merged

Fix xcode paths #1040

merged 7 commits into from
Jul 17, 2019

Conversation

zeroZshadow
Copy link
Contributor

Description

Post process the xcode project library search paths to make sure simulator and arm libraries are separated.

@zeroZshadow zeroZshadow self-assigned this Jul 16, 2019
@improbable-prow-robot improbable-prow-robot added jira/no-ticket Indicates a PR has no corresponding JIRA ticket size/M Denotes a PR that changes 40-149 lines, ignoring generated files. labels Jul 16, 2019
@zeroZshadow zeroZshadow requested a review from jessicafalk July 16, 2019 17:24
Copy link
Contributor

@paulbalaji paulbalaji left a comment

Choose a reason for hiding this comment

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

iOS Device and Simulator connect to local deployments!

Copy link
Contributor

@jamiebrynes7 jamiebrynes7 left a comment

Choose a reason for hiding this comment

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

Add changelog entry pls

@zeroZshadow zeroZshadow requested a review from jamiebrynes7 July 16, 2019 19:46
CHANGELOG.md Outdated Show resolved Hide resolved
Co-Authored-By: Jamie Brynes <[email protected]>
Copy link
Contributor

@jessicafalk jessicafalk left a comment

Choose a reason for hiding this comment

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

as mentioned in slack:
Packages/io.improbable.worker.sdk.mobile/BuildPostProcessXCode.cs(5,19): error CS0234: The type or namespace name 'iOS' does not exist in the namespace 'UnityEditor' (are you missing an assembly reference?)
we need an extra check in case people don't have the iOS module installed

[PostProcessBuild]
public static void OnPostProcessBuild(BuildTarget buildTarget, string path)
{
// Only run for iOS
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need that comment? think that if check is pretty self-explanatory. that's a general comment: some of the comments are good, some of them are not needed like this one or the last one for example

@improbable-prow-robot improbable-prow-robot added size/L Denotes a PR that changes 150-299 lines, ignoring generated files. and removed size/M Denotes a PR that changes 40-149 lines, ignoring generated files. labels Jul 17, 2019
@zeroZshadow zeroZshadow requested a review from jessicafalk July 17, 2019 13:03
Copy link
Contributor

@paulbalaji paulbalaji left a comment

Choose a reason for hiding this comment

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

doubled-checked post-reflection: iOS Device and Simulator still connect to local deployments!

//pbxType = Type.GetType("UnityEditor.iOS.Xcode.PBXProject");
if (pbxType == null)
{
Debug.LogWarning("iOS Support not installed.");
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe also tell them what to do when that happens

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced this with an exception as this should only show up when Unity changes their API. The exception will make sure we catch it in buildkite.

InvokeMethod<string>(xcodeObject, "UpdateBuildPropertyForConfig", configGUID, "LIBRARY_SEARCH_PATHS",
null, new[]
{
"$(SRCROOT)/Libraries/io.improbable.worker.sdk.mobile/Plugins/Improbable/Core/iOS/arm",
Copy link
Contributor

Choose a reason for hiding this comment

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

can we store the string $(SRCROOT)/Libraries/io.improbable.worker.sdk.mobile/Plugins/Improbable/Core/iOS as a field? makes it a bit easier to read

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I rather keep these where they are right now, and it makes their context clear.

Copy link
Contributor

Choose a reason for hiding this comment

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

don't think it would make the context less clear, but fine

InvokeMethod<string>(xcodeObject, "WriteToFile", projPath);
}

private static T InvokeStaticMethod<T>(string methodName, params object[] parameters)
Copy link
Contributor

Choose a reason for hiding this comment

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

that could be factored out into something more generic, but doesn't need to happen now

Removed temp buildkite check
@zeroZshadow zeroZshadow requested a review from jessicafalk July 17, 2019 14:46
@zeroZshadow zeroZshadow merged commit ca92bb5 into develop Jul 17, 2019
@zeroZshadow zeroZshadow deleted the feature/fix-xcode-paths branch July 17, 2019 14:56
jessicafalk pushed a commit that referenced this pull request Nov 15, 2019
* RPC tracking (WIP)

* Tell server to track RPCs

* Add new component to switches

* A few modifications, added TODOs

* Move RPC tracking to SpatialMetrics

* Adjust logs and comments

* RequireSetup

* Use SendEmptyCommandResponse

* Address PR comment

* Apply suggestions from code review

Remove TODOs for queued RPCs (because queuing logic is going away soon)

* Address PR comments
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
jira/no-ticket Indicates a PR has no corresponding JIRA ticket size/L Denotes a PR that changes 150-299 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants