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 GeneratedThirdPartyNotices.txt and make sure it stays in sync #673

Merged
merged 3 commits into from
Apr 4, 2024

Conversation

mikeage
Copy link
Member

@mikeage mikeage commented Apr 4, 2024

No description provided.

@mikeage mikeage requested a review from andybak April 4, 2024 05:09
@mikeage
Copy link
Member Author

mikeage commented Apr 4, 2024

One idea that came up in discord was to get rid of this file and just generate it during the build. If we want to do that, we can make this change:

diff --git i/.github/workflows/build.yml w/.github/workflows/build.yml
index cf543ca25..c61384d9e 100644
--- i/.github/workflows/build.yml
+++ w/.github/workflows/build.yml
@@ -413,6 +413,14 @@ jobs:
           sed -e "s/m_VersionNumber:.*$/m_VersionNumber: $VERSION/" -i Assets/Scenes/Main.unity
           sed -e "s/bundleVersion:.*$/bundleVersion: $VERSION/" -i ProjectSettings/ProjectSettings.asset

+      - uses: actions/[email protected]
+        with:
+          python-version: '3.12'
+
+      - name: Update the ThirdParty NOTICE file
+        run: |
+          python Support/Python/unitybuild/generate_notice.py
+
       - name: Add secure secrets file
         env:
           SECRETS_ASSET: ${{ secrets.SECRETS_ASSET }}

But we should probably keep a stub for local build purposes.

More importantly, it seems like we use the reference in Github as well (look at m_ThirdPartyNoticesURL, referenced in the About option handling in Assets/Scripts/SketchControlsScript.cs, for Mobile only (and note that this is broken on Mac and Linux for now)), so I'm not sure we can get rid of it in git completely. Andy, what do you think?

@mikeage
Copy link
Member Author

mikeage commented Apr 4, 2024

I see that in #644, all platforms use this link.

Perhaps we can generate at build time and push to the docs site, and link to there?

@andybak
Copy link
Contributor

andybak commented Apr 4, 2024

The docs site would be ideal but I'm not averse to a github link if that's easier. Changing the url in #644 is easy so it's your call @mikeage

Not sure of the legalities. If the file isn't in github clones or downloads, but is in our builds - is that enough to satisfy the terms of the Apache license? Or should it be in both.

If so we should maybe run this as a precommit and push responsibility for updating the repo onto the person doing the commit. In which case keep the in-app link as a github link as it already works. I think this was your first suggestion on Discord, wasn't it?

@andybak andybak closed this Apr 4, 2024
@andybak andybak reopened this Apr 4, 2024
@mikeage
Copy link
Member Author

mikeage commented Apr 4, 2024

I don't know about legality. The current method works, and this makes it better in terms of any future changes (honestly unlikely because almost everything is a package and not a ThirdParty directory). But building automatically and updating the docs site (which is less problematic than updating the main repo in an automated fashion) might be even better

@andybak
Copy link
Contributor

andybak commented Apr 4, 2024

I'm slightly unclear where we are specifically with the code in this PR after subsequent discussions. Are you still suggesting it's best to merge this?

@mikeage
Copy link
Member Author

mikeage commented Apr 4, 2024

I don't know :-)

I definitely think we should merge this for the script. As far as the test vs automation... I can see either way. You first raised the Q... so what do you think?

@mikeage mikeage merged commit e23a47a into icosa-foundation:main Apr 4, 2024
57 checks passed
@mikeage mikeage added the infrastructure Build or tooling infrastructure label Apr 4, 2024
@mikeage mikeage deleted the third_party_update branch August 2, 2024 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Build or tooling infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants