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

Remove DLLs build in other repo and download new ones from there #889

Merged
merged 6 commits into from
Sep 3, 2024

Conversation

s09bQ5
Copy link
Collaborator

@s09bQ5 s09bQ5 commented Aug 20, 2024

This is related to UltraStar-Deluxe/mxe#1. It obsoletes #871.

Right now this PR doesn't build because the other repository does not have a release for 67f6a4f9305a2ee1508215ad4a861f23f7b8e3da and Github does not allow unauthenticated downloading of CI artifacts. If someone creates an API token with workflow read permissions, the python script is prepared to use it to download the DLLs from an artifact. When reviewing the python script, take into account that this is the first python script I have ever written. I restricted myself to use only the standard python library functions on purpose.

I still need to check if the license texts need to be updated.

@s09bQ5 s09bQ5 marked this pull request as draft August 20, 2024 09:11
@barbeque-squared
Copy link
Member

The python script looks overall okay (haven't actually run it). The only two (maybe) issues I can see are:

  • you can do sys.exit("this is a more user friendly error message") instead of sys.exit(1). Will (should) still result in exit code 1 but also prints a message.
  • what's the desired behaviour between releases vs latest ci artifact? the usecase would be say ffmpeg8 is released, that'll probably create a PR in both mxe and USDX, but in the meantime other USDX PRs should also still build. My immediate thought would be just making a prerelease ffmpeg8 in mxe and just hardcoding the whatever release URL somewhere in USDX (the python script probably?) but maybe I'm missing some other usecase now. Right now the whole artifact downloading will be irrelevant the moment the first mxe release is published.

@s09bQ5
Copy link
Collaborator Author

s09bQ5 commented Aug 20, 2024

* you can do `sys.exit("this is a more user friendly error message")` instead of `sys.exit(1)`. Will (should) still result in exit code 1 but also prints a message.

The last line output before sys.exit(1) is already "No release matches".

* what's the desired behaviour between releases vs latest ci artifact? the usecase would be say ffmpeg8 is released, that'll probably create a PR in both mxe and USDX, but in the meantime other USDX PRs should also still build. My immediate thought would be just making a prerelease ffmpeg8 in mxe and just hardcoding the whatever release URL somewhere in USDX (the python script probably?) but maybe I'm missing some other usecase now. Right now the whole artifact downloading will be irrelevant the moment the first mxe release is published.

The mxe commit to use when downloading is hardcoded in dldlls.py. Pull requests that want to update DLLs will change that commit id to the one of the corresponding pull request in the mxe repo. Pull requests that don't want to change the DLLs will of course continue to use the commit id that has been committed there previously.
Once a new commit id in dldlls.py has been merged, we make a release in the mxe repo. The name of the release and its tag don't matter. It can be the current date or USDX-v2024.09.0-rc42, etc.
If discussing a pull request changing dldlls.py takes more than 7 days, we can manually trigger a rebuild of the artifact in the mxe repo. Unfortunately pull requests from people without write access to this repository will not be able to use a token for accessing artifacts that we configure as a secret for this repo. But I guess non-collaborators will rarely try to update the DLLs.

The unit got commented out in the .dpr file in 2015.
For the record:

BASS 2.4.12
BASS_FX 2.4.11.1
GLEW 1.5.3
OpenCV 2.1
MESA 11.0.5
projectM 1.1
@s09bQ5 s09bQ5 marked this pull request as ready for review August 25, 2024 15:52
@s09bQ5
Copy link
Collaborator Author

s09bQ5 commented Aug 25, 2024

Had to tweak the python script a little. Downloading from workflow artifacts is now implemented correctly.

Can someone with sufficient permissions (not an "Outside Collaborator" like me) create a fine grained personal access token that has read-only actions permissions for the mxe repository? The token will automatically be granted read-only metadata permissions. The maximum expiration date is one year from now.

Then please add this token as a repository secret to the USDX repository and name it MxeActionsReadAccessToken.

@bohning
Copy link
Collaborator

bohning commented Aug 25, 2024

I do not have the permissions to do that. @basisbit could you please enable setting personal access tokens or give me the permissions to do so (https://docs.github.com/en/organizations/managing-programmatic-access-to-your-organization/setting-a-personal-access-token-policy-for-your-organization).

@s09bQ5
Copy link
Collaborator Author

s09bQ5 commented Sep 1, 2024

@basisbit ?

@bohning
Copy link
Collaborator

bohning commented Sep 2, 2024

@s09bQ5 Could you contact me on Discord?

@s09bQ5
Copy link
Collaborator Author

s09bQ5 commented Sep 3, 2024

A token has been added as a secret. The CI is now able to use DLLs from workflow artifacts.

@bohning bohning merged commit 579a89c into master Sep 3, 2024
9 checks passed
@bohning bohning deleted the download-dlls branch September 3, 2024 13:39
@s09bQ5
Copy link
Collaborator Author

s09bQ5 commented Sep 3, 2024

We still need to work out the best way to handle DLL updates. I suggest:

  1. make a PR in the MXE repo
  2. make a PR in the USDX repo that updates the commit id in dldlls.py
  3. once everyone is happy, merge the PR in the MXE repo
  4. update the PR in the USDX repo to point to the commit id of the merge request in the MXE repo
  5. merge the PR in the USDX repo
  6. make a pre-release in the MXE repo and attach a zip file with the DLLs from the CI run
  7. when a release is made in the USDX repo, rename the corresponding release in the MXE repo and remove the pre-release marker

@DeinAlptraum
Copy link
Contributor

DeinAlptraum commented Sep 9, 2024

Haven't looked too deeply into this, but seems like this breaks when trying to use it with PRs from forks. They don't get access to secrets iirc, see this build

@s09bQ5
Copy link
Collaborator Author

s09bQ5 commented Sep 9, 2024

Hmm, enumerating and downloading releases works without authentication. Maybe the environment variable is set to an empty string and leads to an Authorization header without token?

@DeinAlptraum
Copy link
Contributor

Just checked https://docs.github.com/en/actions/security-for-github-actions/security-guides/using-secrets-in-github-actions#using-secrets-in-a-workflow

With the exception of GITHUB_TOKEN, secrets are not passed to the runner when a workflow is triggered from a forked repository.

and

If a secret has not been set, the return value of an expression referencing the secret (such as ${{ secrets.SuperSecret }} in the example) will be an empty string.

So that does seem to be the case.

Is there any reason we can't e.g. wget "https://github.com/UltraStar-Deluxe/mxe/releases/latest/download/usdx-dlls-i686-v2023.12.0.zip" or similar?
I've just tested this and it worked fine

@s09bQ5
Copy link
Collaborator Author

s09bQ5 commented Sep 9, 2024

We can, but the enumeration of the releases to determine the download link for the commit id fails because there is an invalid Authorization http header. The condition in dldlls.py that adds the header needs to be fixed.

But maybe we do want to add the GITHUB_TOKEN instead because there is a huge difference in the number of API calls that can be made per hour with and without authorization. GITHUB_TOKEN won't work with workflow artifacts, though.

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.

4 participants