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

VLC2: add a fix for < 10.6 #26144

Merged
merged 1 commit into from
Dec 13, 2024
Merged

Conversation

barracuda156
Copy link
Contributor

Description

Fix some issues which prevented it building on 10.5.8.

No need in revbump, since all changes are only used on < 10.6, where it did not build before.

Type(s)
  • bugfix
  • enhancement
  • security fix
Tested on

macOS 10.5.8
Xcode 3.1.4

Verification

Have you

  • followed our Commit Message Guidelines?
  • squashed and minimized your commits?
  • checked that there aren't other open pull requests for the same change?
  • referenced existing tickets on Trac with full URL in commit message?
  • checked your Portfile with port lint --nitpick?
  • tried existing tests with sudo port test?
  • tried a full install with sudo port -vst install?
  • tested basic functionality of all binary files?
  • checked that the Portfile's most important variants haven't been broken?

@barracuda156
Copy link
Contributor Author

@reneeotten @herbygillot Bump

@reneeotten
Copy link
Contributor

there is way too much patching involved here for me to review. If you say this is older, upstream code then please add links to PRs/commits where these changer weee made which you are reverting here.

@barracuda156
Copy link
Contributor Author

@reneeotten It is every time astonishing how other commits get merged without testing anything beyond 3 latest macOS versions (and often breaking builds on multiple systems), while a fix literally used only for a few old systems all of a sudden requires such a scrutiny. That too, this is a port which exists for older systems and maintained by me…

AudioComponent patch reverts this for 10.5 and earlier: videolan/vlc@5e59f5f
SystemConfiguration patch borrows from here: https://raw.githubusercontent.com/videolan/vlc/adb1e78b73477c01481927ce8c1d6202fb890233/src/darwin/netconf.c

@reneeotten
Copy link
Contributor

You are always asked to put links in patchfiles, so just do that from the beginning and I don't have to request it again.

We "officially" only support the latest three releases as you are well aware. So if it works there it's good enough to merge; anything else is for interested parties to take care of. But these interested parties will have to go the extra mile to provide proper documentation/links to show what is done and why so that others -not as versed in older OS releases- have a way of following it. Both at the time of a PR and two years down the line.

So please just do as requested many times before or don't tag me anymore in PRs.

@tobypeterson
Copy link
Contributor

The memset in patch-SystemConfiguration.diff looks very wrong, seems like it should be clearing buffer, not host_buffer.

Otherwise, these changes look fine, though I personally really dislike OS-specific patches. Patches that compile everywhere can be submitted upstream.

@barracuda156
Copy link
Contributor Author

@tobypeterson This version is not maintained by upstream, we keep it only because Qt4 support it killed after (and because it works fine and has some GUI and not just CLI).

@markemer
Copy link
Member

markemer commented Dec 5, 2024

I have nothing as old as Darwin 16.x or earlier, and this is always gonna show green on CI - I have no objection, but I certainly have no idea if this is correct or not - I'd like to prioritize ancient (10.12) over antediluvian (<10.6) versions of macOS / OSX. But if the build bot dies on these we could always roll it back.

@tobypeterson
Copy link
Contributor

Sure, this change clearly only affects ancient versions of macOS, so it's probably fine regardless. Still, the memset call in the patch is clearly wrong, so I'd at least want that to be fixed first.

@barracuda156
Copy link
Contributor Author

@markemer As I noted above,

all changes are only used on < 10.6

This has no impact even on 10.6, since patches are only applied prior to that. So whether VLC2 builds on 10.12 or not, this PR is inconsequential.

@barracuda156
Copy link
Contributor Author

Still, the memset call in the patch is clearly wrong

@tobypeterson If you suggest a change, I will add it.

@tobypeterson
Copy link
Contributor

I think that memset line is not necessary - if you look at netconf.c, it is correctly called a few lines earlier already.

I also see that the patch wraps that code in #if TARGET_OS_IPHONE so it probably doesn't actually matter, but it's still gross. If you remove that bit from the patch, I am fine with this PR.

@barracuda156
Copy link
Contributor Author

@tobypeterson I have updated the patch to drop that memset line.

@tobypeterson tobypeterson merged commit 3912b3d into macports:master Dec 13, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

5 participants