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

Add visionOS Support #384

Merged
merged 5 commits into from
Jul 9, 2024
Merged

Add visionOS Support #384

merged 5 commits into from
Jul 9, 2024

Conversation

PSchmiedmayer
Copy link
Contributor

Add visionOS Support

Thank you @philprime & team for the work on TPPDF; nice Swift Package and great functionality!

♻️ Current situation & Problem

  • The current package is supporting iOS and macOS targets.
  • The underlying infrastructure is fully supporting visionOS and only needs requires modifications to the #if statements.
  • The current sample application's progress bar is not using constraints for the alignment and uses an outdated UIWebView that is not available on visionOS.

⚙️ Release Notes

  • Adds visionOS support to TPPDF
  • Improves example app to use constraints & WKWebView
  • Update Github Actions with vision build

📚 Documentation

  • Updates the README with links to the Swift Package Index

✅ Testing

@PSchmiedmayer
Copy link
Contributor Author

@philprime Thank you for triggering the builds. Seems like the fact that the Danger GitHub Action is relying on a secret not available outside of the repo is failing the Danger check for forks: https://github.com/PSchmiedmayer/TPPDF/blob/main/.github/workflows/danger.yml#L20.

Might be resolved by just passing in the default GITHUB_TOKEN which has limited access and permissions even in forked repos (can be changed in the repo settings). Let me know if you want me to modify the GitHub Action or how to proceed with the merge checks here.

@philprime
Copy link
Member

Hi @PSchmiedmayer :)

Thank you for the PR and the time you invested into it!

I just came back from vacation, so I didn't have the time yet to look into it in detail, but I'll do it as soon as possible.

@philprime
Copy link
Member

Thanks again for your PR.
I appreciate the work you put into it!

Regarding the changes in code:
Looks good, lets get this merged.

Regarding the CI matrix:
Looks good to me, but we need simpler checks names, otherwise the required status configuration will require more maintenance. We should be able to set the job name of the GitHub Action based on the matrix - you can find an example here, where I create Docker images based on a matrix:

https://github.com/kula-app/containers/blob/bc111124dc55d20c2050b5d8c229e34837027208/.github/workflows/build-images.yml#L19

Would appreciate if you could add this to your PR.

Regarding Danger, the access token with limited access from our bot user @techprimate-bot as defined in their documentation.

But I went ahead and created a test fork and PR to check if GITHUB_TOKEN works too. Looking at the logs it seems like the default token did not have access to create the Danger comment in the PR/Issue.

Request failed [403]: https://api.github.com/repos/techprimate/TPPDF/issues/385/comments
Response: {
  "message": "Resource not accessible by integration",
  "documentation_url": "https://docs.github.com/rest/issues/comments#create-an-issue-comment",
  "status": "403"
}
Feedback: undefined

Looking at the documentation for permissions of the GITHUB_TOKEN, I found the following section:

You can use the permissions key to add and remove read permissions for forked repositories, but typically you can't grant write access. The exception to this behavior is where an admin user has selected the Send write tokens to workflows from pull requests option in the GitHub Actions settings. For more information, see "Managing GitHub Actions settings for a repository."

After looking further it, and noticing that even setting additional permissions like the following, it did not work:

jobs:
  job-danger:
    permissions:
      actions: read
      attestations: read
      checks: read
      contents: read
      deployments: read
      discussions: read
      id-token: write
      issues: write
      packages: read
      pages: read
      pull-requests: write
      repository-projects: read
      security-events: read
      statuses: read

I guess we need to skip Danger in forked PRs.
If you have another idea, would be happy to read it.

@PSchmiedmayer
Copy link
Contributor Author

@philprime Thanks for the review!

Good idea! I update the GitHub Action Name to just use the matrix.build.sdk name which might be the most expressive of the ones within our matrix configuration.

Regarding the danger check: Unfortunate but I don't have any other suggestions as well. We have similar challenges with some of our merge checks in our open-source repos.

@philprime
Copy link
Member

Thanks for the adaptation.

Super-small request, just for the sake of completeness: could you please also change the macOS Unit Test to use the same format?

This is now with the matrix naming:

Tests / Unit Tests (iphonesimulator)
Tests / Unit Tests (xros) (pull_request)
Tests / macOS Unit Tests (pull_request) 

but this would be nicer:

Tests / Unit Tests (iphonesimulator)
Tests / Unit Tests (xros)
Tests / Unit Tests (macos)

I don't want to picky, I just think this could be a good point in time for a small change in the PR.

You can either rename the macOS job to use this format, or create a matrix with a single entry. Both fine to me.

Thank you!

@philprime
Copy link
Member

Also regarding Danger (sent my comment to fast):

Looking at my test PR #385 we can add a fallback for the Danger GITHUB_TOKEN, so at least it soft-fails:

        with:
          args: --failOnErrors --no-publish-check
        env:
-         GITHUB_TOKEN: ${{ secrets.DANGER_GITHUB_API_TOKEN }}
+         GITHUB_TOKEN: ${{ secrets.DANGER_GITHUB_API_TOKEN || secrets.GITHUB_TOKEN }}

I actually believe there is a bug in Danger, that it should actually fail (danger/danger-js#1441) but for me it is not critical enough to block your PR.

So please add the GITHUB_TOKEN fallback, and we'll fix Danger for forks another time.

@PSchmiedmayer
Copy link
Contributor Author

@philprime Sure; I updated the GitHub Action names & Danger job, thanks for the input. I like the idea to keep this clean 🚀

The jobs will now be named:

Tests / Unit Tests (iphonesimulator) (pull_request)
Tests / Unit Tests (xros) (pull_request)
Tests / Unit Tests (macos) (pull_request) 

The pull_request postfix seems to be automatically added by the trigger of the action.

@philprime
Copy link
Member

Fantastic. Thanks for the additional effort.

This PR looks good to me so I'll go ahead and merge it, as well as release a new version afterwards

@philprime philprime merged commit 0916bd3 into techprimate:main Jul 9, 2024
6 checks passed
@PSchmiedmayer
Copy link
Contributor Author

Thank you @philprime 🚀

Thank you for creating and maintaining this framework; always happy to support fellow open-source work 👍

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.

None yet

2 participants