PSchmiedmayer commented 4 months ago

Add visionOS Support

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

:recycle: 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.

:gear: Release Notes

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

:books: Documentation

Updates the README with links to the Swift Package Index

:white_check_mark: Testing

Added build action to test building on visionOS
Executing the tests on visionOS is unfortunately currently timing out due to a lack of support by the GitHub provided images. More context can be found in https://github.com/actions/runner-images/issues/9511

PSchmiedmayer commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

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 commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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 (https://github.com/danger/danger-js/issues/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 commented 3 months ago

@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 commented 3 months ago

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

PSchmiedmayer commented 3 months ago

Thank you @philprime 🚀

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

techprimate / TPPDF