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

[VIDEO-3086] Configure gst-plugins-rs source, ref #18

Merged
merged 6 commits into from
Oct 28, 2024

Conversation

btgoodwin
Copy link

@btgoodwin btgoodwin commented Oct 24, 2024

Updated the github action to add recipes_remotes and recipes_commits into the configuration file, pointing gst-plugins-rs at our repository. The target including the following two changes is 0.12.9-lldc.1, here.

  1. [VIDEO-3085] Add extra event forwarding to intersink element gst-plugins-rs#2
  2. [VIDEO-3092] (Resurrected) Backport awss3sink softseek from upstream PR gst-plugins-rs#5

@btgoodwin btgoodwin force-pushed the video-3086-configurable-gst-plugins-rs-source branch from a504b93 to 3acc266 Compare October 25, 2024 12:37
This patch release includes our aws3sink w/ cache and intersink changes
@btgoodwin btgoodwin marked this pull request as ready for review October 25, 2024 15:07
@btgoodwin btgoodwin added enhancement New feature or request github-actions Pull requests that update GitHub Actions code needs-code-review Code review needed from developer(s) labels Oct 25, 2024
@btgoodwin btgoodwin requested a review from jawilson October 25, 2024 15:09
@btgoodwin
Copy link
Author

I'm stumped. Earlier I let it run with a different existing tag and it was fine. Now it's not with the new tag. Their readme says "branch"...which won't be great; the variable is called commits after all.

The spec for git reset --hard is <commit>, which does accept a remote name but goes to the head of the branch (`remote/branch` format).  If we need to target a tag, our tag must be unique.
@btgoodwin
Copy link
Author

Look'n good. It made it past the fetch step @jawilson. Please let me know if this is implemented how you would have preferred. I used a pair of input variables to tie this back. The one for the ref can either be the lldc/<branch name> or a tag that's unique to our repository, as is currently set in this PR.

Copy link
Member

@jawilson jawilson left a comment

Choose a reason for hiding this comment

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

We'll need to also update the config-hash calculation. I'd suggest something simple like:

config_hash=$(echo "${CERBERO_ARGS}${{ inputs.config }}${GST_PLUGINS_RS_SOURCE}${GST_PLUGINS_RS_REF}" | sha256sum | cut -d' ' -f1)

.github/actions/build-gstreamer-for-windows/action.yml Outdated Show resolved Hide resolved
.github/actions/build-gstreamer-for-windows/action.yml Outdated Show resolved Hide resolved
@btgoodwin btgoodwin requested a review from jawilson October 28, 2024 16:09
Copy link
Member

@jawilson jawilson left a comment

Choose a reason for hiding this comment

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

LGTM assuming the build passes.

@btgoodwin btgoodwin removed the needs-code-review Code review needed from developer(s) label Oct 28, 2024
@btgoodwin
Copy link
Author

@jawilson Well, it's through the wickets but I can't merge to the branch because it's protected. I assume you have permission?

@jawilson
Copy link
Member

@jawilson Well, it's through the wickets but I can't merge to the branch because it's protected. I assume you have permission?

Got a little carried away with the branch protection rulesets. I couldn't merge either. Updated and good to merge now.

@jawilson jawilson merged commit 4361ec5 into 1.24-lldc Oct 28, 2024
1 check passed
@jawilson jawilson deleted the video-3086-configurable-gst-plugins-rs-source branch October 28, 2024 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request github-actions Pull requests that update GitHub Actions code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants