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

Update for work state to 2023.08.01 #122

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

coordinator/coordinator.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@nanoscopic nanoscopic left a comment

Choose a reason for hiding this comment

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

Mostly looks fine. Have a few questions but I don't see anything here that is functionally problematic.

I'm not clear on why view_log and wda_wrapper have been moved into subdirectories, but I also have no issue if that is done.

The documentation changes are quite messy, and a lot of that documentation needs to be cleaned up / revamped. It was though in bad shape to begin with, so I'm not really concerned.

Are there PRs in other repos associated with this PR?

@@ -1,13 +1,12 @@
module coordinator.go

go 1.12
go 1.16
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see any issue with updating the base requirement to 1.16. I am curious if there is any specific need to do so?

Copy link
Author

Choose a reason for hiding this comment

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

This file was generated on version 16, left it as it is.
Reason - 1.12 is deprecated and more unavailable in Homebrew:
https://formulae.brew.sh/formula/go#default

1.16 now the most wounding available version

@@ -115,7 +115,7 @@ func proc_device_ios_unit( o ProcOptions, uuid string, curIP string) {
return true
}
o.procName = "stf_device_ios"
o.binary = "/usr/local/opt/node@12/bin/node"
o.binary = "/usr/local/opt/node@14/bin/node"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does latest STF now require node 14? I set it this way just because it was what was installed on my machine. Really this should be changed to auto-detect which Node is installed and use the latest acceptable version. ( which may not be the latest if STF itself doesn't support the latest. )

Copy link
Author

Choose a reason for hiding this comment

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

node@12 now is more unavailable in Homebrew:
https://formulae.brew.sh/formula/node#default

For this reason, the version is raised to the minimum accessible and check working provider on node@14.

Current used version NodeJS in STF view 17:
https://github.com/DeviceFarmer/stf/blob/master/Dockerfile-debian-x86_64#L29

I think that updating the version of NodeJs is actually better done as part of a separate task. There is a PR about this:
#118

# Deployment

## Relevance
This instruction actual on 2023/08/09
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this date here for? People can see when the docs were last updated by looking at the commit date.

Copy link
Author

Choose a reason for hiding this comment

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

Fix it.

This instruction actual on 2023/08/09

## Tested working configuration
- PC: MacMini 2018 (x86_64)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see how the physical hardware matters. It would not change anything.

Copy link
Author

Choose a reason for hiding this comment

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

remove it


## Tested working configuration
- PC: MacMini 2018 (x86_64)
- OS: MacOS Ventura 13.5
Copy link
Collaborator

Choose a reason for hiding this comment

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

No part of MacOS is used. MacOS in the sense of the repo is only used as a Linux environment really that has usbmuxd running. In MacOS case it is the Apple usbmuxd implementation, so this could make some small difference, but I don't think is worth mentioning.

Copy link
Author

Choose a reason for hiding this comment

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

The MacOS version can affect the available Homebrew packages and the available XCode version.
For this reason, it is indicated

Now libusbmuxd dont compile on MacOS 12 and requires MacOS > 13.0 and new XCode.

Fix info

- PC: MacMini 2018 (x86_64)
- OS: MacOS Ventura 13.5
- IDE: XCode 14.3.1
- Mobile devices: iPhone and iPad different years with iOS versions: 14.1, 14.2, 14.6, 14.7, 15.0, 15.4.1, 16.6
Copy link
Collaborator

Choose a reason for hiding this comment

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

A very wide set of device versions beyond this has been tested. It long since became irrelevant to list which versions have been tested. If versions don't work they should just have an open issue.

Copy link
Author

Choose a reason for hiding this comment

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

remove it

1. Click `+` in the lower left corner
1. Select `Apple Development`
1. Install [Homebrew](https://docs.brew.sh/Installation)
1. Install Python 2.7 from MacPorts
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think I ever said this. Why install Python from MacPorts?

Copy link
Author

Choose a reason for hiding this comment

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

Python 3 is default from MacOS 11:
https://developer.apple.com/documentation/macos-release-notes/macos-catalina-10_15-release-notes#Deprecations

To use Python 2.7, you need to install it separately. Homebrev does not support the installation of Python, so MacPorts remain the most convenient way.

1. Download actual profiles for device by Xcode or TestFlight:
https://docs.fastlane.tools/actions/sigh/

## 4. Prepare dependenses on build machine
Copy link
Collaborator

Choose a reason for hiding this comment

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

MIsspelling. It is spelled "dependencies"

Copy link
Author

@den-patrakeev den-patrakeev Aug 14, 2023

Choose a reason for hiding this comment

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

Thanks, fix it


In Homebrew no formulae for it: https://github.com/Homebrew/homebrew-core/pull/87059

1. Resolve by: https://github.com/libimobiledevice/libimobiledevice/issues/1217
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this pile of junk really needed??

Copy link
Author

Choose a reason for hiding this comment

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

Fix it

}
```

1. Fix [error](https://github.com/DeviceFarmer/stf_ios_support/issues/100) in repos `ios_video_pull`
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be addressed by having a mentioned PR for that repo also that gets accepted along with this one.

Copy link
Author

Choose a reason for hiding this comment

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

@den-patrakeev
Copy link
Author

Mostly looks fine. Have a few questions but I don't see anything here that is functionally problematic.

I'm not clear on why view_log and wda_wrapper have been moved into subdirectories, but I also have no issue if that is done.

The documentation changes are quite messy, and a lot of that documentation needs to be cleaned up / revamped. It was though in bad shape to begin with, so I'm not really concerned.

Are there PRs in other repos associated with this PR?

Add new PR by comment: dryark/ios_video_pull#2

@den-patrakeev
Copy link
Author

@nanoscopic hi!
Please get acquainted.

@den-patrakeev
Copy link
Author

@nanoscopic remind me of PR

@nm-free
Copy link

nm-free commented Dec 11, 2023

@nanoscopic hello

When will it be reviewed and merged?

@volvoeacs
Copy link

@nanoscopic , I am trying to setup iOS provider in Mac mini m2 , Mac Samoa OS. Do I need to take care of anything extra to make it work with arm based Mac system?

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.

5 participants