#47192: update: VLC 2.2.0 --------------------------+-------------------------------- Reporter: rjvbertin@… | Owner: macports-tickets@… Type: update | Status: new Priority: Normal | Milestone: Component: ports | Version: Resolution: | Keywords: haspatch Port: VLC | --------------------------+-------------------------------- Comment (by rjvbertin@…): Replying to [comment:3 ryandesign@…]:
You've made a lot of changes and obviously spend a lot of time on this, so thank you. I see a few problems though.
Define few? :) Some quick reactions before I address matters when I have a moment for that:
You've removed the `# $Id$` line which we want to keep, and have added nonstandard and in some cases redundant items to the modeline. I would stick with the standard modeline. If you
I was under the impression the the Id line was added automatically during a commit, and thought it'd help avoid confusion. Should have realised it also helps in comparing versions... I don't think I changed the standard modeline. I added one for a different editor, one I happen to be using (in fact I use KDevelop). I think it can be moved to EOF if that's less invasive, I'll want to keep it as long as my name is in the maintainer list. I don't think it'd be possible (nor a good idea) to merge the 2 modelines. BTW, the vi modeline somehow interferes with my own vim settings, causing files to be marked "modified" as soon as I open them in the editor. I've asked about this on the ML but never got an answer, so I'm re-raising the issue here. It's rather annoying.
You patch does not apply. The `# $Id$` line you removed says your changes are based on r126549 from October of 2014, but the port has been updated several times since then. For example, a qt4 variant has already been added.
The Portfile.diff patch? Evidently that one applied when I uploaded my attachments, and parallel changes to the reference are a reason I don't like to upload only diffs.
I don't believe you've declared the port conflicts correctly. In the libVLC subport you've declared correct conflicts on VLC, VLC-devel and libVLC-devel, and in the next block, which takes effect for VLC, you've declared conflicts on libVLC, libVLC-devel and VLC-devel, but none of the preceding actually gets used, because a few lines later you overwrite the `conflicts` line and set it to just VLC, so now VLC conflicts with itself.
That'd be an oversight...
I'm not very comfortable with VLC and libVLC conflicting with each other. If we remove VLC and keep just libVLC, as you suggested, then it's not a problem, but if we keep both, then it sounds like it would make more sense for VLC to depend on libVLC (and for VLC to not install the files that libVLC installs), rather than conflicting with it.
That would be the other approach. The reason I didn't follow it is that, as you probably saw, there is no way to build just libVLC, and just the player. Things are conceived to build the whole thing, and for port:libVLC I simply throw away the unused things in the post-destroot. All of which means that if someone wants to install port:VLC and for some reason has to build from source, s/he will find it takes about twice as long if we make VLC depend on libVLC.
It makes sense to iron out these problems in the VLC port first, but once we're happy with the changes, we would want to make the same changes to the VLC-devel port; -devel and non-devel ports should be kept as similar as reasonably possible.
I can't recall nor check right now if I uploaded my own VLC-devel port, which now is a fork of this port and builds VLC 3 from git.
You've added code to enable hfsCompression during extraction. I think that's something we should do in MacPorts base, not in individual ports.
I agree. And I'll promise to remove the block as soon as it becomes redundant, but in the meantime there's no harm in keeping it, right?
You've disabled FreeRDP support on Snow Leopard, with a comment that FreeRDP does not build on Snow Leopard. Is there a ticket for that? FreeRDP does not build on any system right now, because FreeRDP is not compatible with cmake 3.1 or later (#47389); perhaps that's what you were seeing. Perhaps this would be a good reason not to enable FreeRDP support yet.
I didn't file a ticket. I apparently tried to build libVLC on my 10.6 VM but if so that was just to satisfy a dependency so I didn't spend more time on it than strictly necessary. It's perfectly possible that I ran into the cmake issue (I built FreeRDP on 10.9 using cmake 3.0.2). FreeRDP support is probably rather ... esoteric in libVLC anyway so yeah, I'll just disable it.
You've rearranged some existing lines which use `file copy` and `file delete -force`. We may as well clean up those lines: `file copy` can be replaced with `copy`, and `file delete -force` can be replaced with `delete`. `file rename` can be changed to `move`.
The guide isn't explicit on this: does `delete` have an implicit `-force`?
You install a file from the files directory which contains the hardcoded path /Applications/MacPorts, then check in the portfile whether ${applications_dir} equals /Applications/MacPorts, and if not, use `reinplace` to fix it. This works, but is not the way we usually do it. Instead, when there is a value that could vary, we put a placeholder in the file. In this case, the placeholder could be `@APPLICATIONS_DIR@`. Then in the portfile you would use `reinplace` to replace that placeholder with the variable, and would not need to conditionalize it.
I'm not really sure why I'm conditionalising it in the first place; reinplace is a noop the 2nd time it's called, no? I know the official way is to use a placeholder, but the pragmatist I am sees just 2 perfectly equivalent replacement patterns where one can in fact be tested "as is". :) -- Ticket URL: <https://trac.macports.org/ticket/47192#comment:4> MacPorts <https://www.macports.org/> Ports system for OS X