Skip to content

Update dependency provider#67

Merged
Noarkhh merged 5 commits intomasterfrom
update-dependency-provider
Oct 1, 2025
Merged

Update dependency provider#67
Noarkhh merged 5 commits intomasterfrom
update-dependency-provider

Conversation

@Noarkhh
Copy link
Copy Markdown
Contributor

@Noarkhh Noarkhh commented Sep 29, 2025

No description provided.

@Noarkhh Noarkhh requested a review from mat-hek as a code owner September 29, 2025 10:48
@Noarkhh Noarkhh force-pushed the update-dependency-provider branch from b876715 to 1d08204 Compare September 29, 2025 10:48
@Noarkhh Noarkhh force-pushed the update-dependency-provider branch from 6c4d1ec to cf22863 Compare September 29, 2025 12:08
@mat-hek
Copy link
Copy Markdown
Member

mat-hek commented Sep 29, 2025

@Noarkhh why fix on this specific version?

@Noarkhh
Copy link
Copy Markdown
Contributor Author

Noarkhh commented Sep 30, 2025 •
edited
Loading

@mat-hek my thought process was that this would prevent from unexpected changes in the behavior of libopus, which could be updated independently, but on the other hand we control the releases, so it could be more handy to just take the latest version

@mat-hek
Copy link
Copy Markdown
Member

mat-hek commented Sep 30, 2025

@Noarkhh Actually, you could've been right, because if libopus ever makes a breaking change and we precompile it and adjust the plugin, all the older versions of the plugin will stop working, as they'll download latest. If those libs used semver, we could use elixir's ~> syntax and Version module to resolve the version, but I'm not sure they do 🤔

Also, I thought that it may be useful to be able to override the version of a given lib in the config. That could be supported by the provider.

@Noarkhh
Copy link
Copy Markdown
Contributor Author

Noarkhh commented Sep 30, 2025

@mat-hek I think they generally do, but not always, so as using semver would be nice I think it could become problematic.

@Noarkhh Noarkhh force-pushed the update-dependency-provider branch from 458df68 to af3340a Compare September 30, 2025 12:18
@Noarkhh Noarkhh self-assigned this Sep 30, 2025
mat-hek
mat-hek reviewed Sep 30, 2025
View reviewed changes
Comment thread test/membrane_opus/encoder_test.exs
@reference_path "test/fixtures/encoder_output_reference"

defp setup_pipeline(output_path) do
on_exit(fn -> File.rm(output_path) end)
Copy link
Copy Markdown
Member

@mat-hek mat-hek Sep 30, 2025

Choose a reason for hiding this comment

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

Should thie be removed?

Copy link
Copy Markdown
Contributor Author

@Noarkhh Noarkhh Sep 30, 2025

Choose a reason for hiding this comment

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

this removes the output file from tmp_dir location, I don't think it's necessary

@Noarkhh Noarkhh force-pushed the update-dependency-provider branch from 896f7b8 to 513b08a Compare October 1, 2025 08:14
@Noarkhh Noarkhh merged commit 2b9006d into master Oct 1, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mat-hek mat-hek mat-hek approved these changes

Assignees

@Noarkhh Noarkhh

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Noarkhh @mat-hek