[OS X]: adaptations for VLC built the MacPorts way
MacPorts has ports for VLC (2.1.5) and VLC-devel (currently 2.2.0 rc2) which install the components of interest to other ports in Linux/Posix style under its install prefix (typically /opt/local), and alter the VLC.app app bundle such that these components are found through symlinks to the directories in which they are installed:
VLC.app/Contents/MacOS/lib -> ${prefix}/lib VLC.app/Contents/MacOS/plugins -> ${prefix}/lib/plugins
VLC.app/Contents/MacOS/share and /opt/local/share/vlc have slightly different contents so I have left them as is for now.
VLC 2.1.5 had issues with this scheme for instance when calling the non app-bundle executable ${prefix}/bin/vlc, or when loading libvlc in any 3rd party application like phonon-backend-vlc . The plugins weren't found. This has been remedied to a large extent by Rémy Denis-Courmont in darwin/dirs.c ; attached are the remaining changes I have found necessary. Though perfectly transparent, I'm not expecting these changes to be incorporated. It would be useful though, and I would be very much interested in constructive feedback how to improve them (or the MacPorts install scheme).
Note that these don't make ${prefix}/bin/vlc functional beyond finding its plugins, at least not with the standard OS X interface module (DBus control appears to work, though). Also note that the executable built this way has an issue initialising OpenGL, which is not due to the attached patches.
Activity
added Component::Core Severity::normal Type::feature Version::master git labels
changed milestone to %Bugs paradize
Hello,
I'd be interested in merging your patch. However, please split it between the darwin-specific core file and the lua one. Further, please think of a way to avoid introducing a global variable just for "libDirIsPosix".
Regarding the lua patch, please reduce the 3 line comment to something like "check if installed outside the regular app bundle" or something like that.
Rest LGTM.
-
Great!
Do you have any suggestions for avoiding the (static) global variable libDirIsPosix? Would it be OK and preferable to reintroduce a static worker function for config_GetLibDir which takes an additional argument, use that in dirs.c, and replace config_GetLibdir with a proxy that uses a stub for the IsPosix argument?
Original author: rene j.v. bertin
-
Replying to [comment:6 jb]:
OK for non-static, it's your code. But in that case I'd appreciate if you told me how to name the function (and what type to give the IsPosix argument), because I don't want to perpetuate a trial-and-error process trying to get things done exactly the way you want. :)
The plugins should be taken next to libvlccore.dylib, whether it's in the Application bundle of /usr/local/ Moreover, there should be no issue of listing both. VLC deals with this correctly.
May I point out that that's not how things are (typically) done under Linux? (I'm using Ubuntu for typical here.). And I'm not aware I'm instructing configure to install the plugins in a separate directory from libvlccore; I presume they end up in lib/vlc/plugins because that's where they end up on Linux.
And you really should split your patches. There are grammar fixes in there, too.
You really expect a separate patch for typo and grammar fixes? This is not a ReviewBoard, so I'll split up my patches when the time comes :)
Original author: rene j.v. bertin
Replying to [comment:7 rene j.v. bertin]:
The plugins should be taken next to libvlccore.dylib, whether it's in the Application bundle of /usr/local/ Moreover, there should be no issue of listing both. VLC deals with this correctly.
May I point out that that's not how things are (typically) done under Linux? (I'm using Ubuntu for typical here.). And I'm not aware I'm instructing configure to install the plugins in a separate directory from libvlccore; I presume they end up in lib/vlc/plugins because that's where they end up on Linux.
Sure, libvlc.so is in /usr/lib/ and vlc plugins are next to libvlccore.so, after you add vlc/plugins
On Windows you just add plugins/
And you really should split your patches. There are grammar fixes in there, too.
You really expect a separate patch for typo and grammar fixes?
Yes.
-
Replying to [comment:8 jb]:
Sure, libvlc.so is in /usr/lib/ and vlc plugins are next to libvlccore.so, after you add vlc/plugins
So we agree on :
lib/libvlc lib/libvlccore lib/vlc/plugins? If so, nothing more to be seen in that subtopic... :)
You really expect a separate patch for typo and grammar fixes?
Yes.
Ok, you'll get that once the main part of the patches has been dealt with. Or you can just do the corrections directly. I'm not going to open a dedicated ticket for something as trivial as a typo, grammar or whitespace correction, that's a waste of everyone's time and resources.
Original author: rene j.v. bertin
-
-
-
Replying to [comment:1 fkuehne]:
Regarding the lua patch, please reduce the 3 line comment to something like "check if installed outside the regular app bundle" or something like that.
That part is not actually checking anything, so I added a slightly different 1-liner comment.
I changed darwin/dirs.c as discussed above, introducing a non-static getLibDir() worker function. I still think it'd be cleaner to make it static, like getAppDependentDir lower down in the same file.
I've removed the English corrections. It's too much of a hassle to whip up a dedicated patch for them.
Original author: rene j.v. bertin
-
The first part of the dir patch is totally wrong:
- comments are bad (DATE, seriously?)
- changing style for no reason
- adding case functions for no reason
- Add MacOS, while this is a term that should get away, and we remove from the codebase.
- uses FALSE/TRUE, and ints for Boolean!
And all this just to set a boolean to true.
The second part, changes the data() place completely and is way too complex to just add a new path folder, that could be defined at compile time.
-
Replying to [comment:1 fkuehne]:
I'd be interested in merging your patch.
Hello fkuehne: if you're still interested in merging my patches, please be inspired and do what you seem fit. I give up trying to comply with the whims of someone who is apparently on a personal crusade against everything MacPorts related.
Original author: rene j.v. bertin
-
Replying to [comment:14 rene j.v. bertin]:
Replying to [comment:1 fkuehne]:
I'd be interested in merging your patch.
Hello fkuehne: if you're still interested in merging my patches, please be inspired and do what you seem fit. I give up trying to comply with the whims of someone who is apparently on a personal crusade against everything MacPorts related.
Wow, that's low.
I spent time to review your patches, and this is your answer, instead of improving them?
Especially after I told you how to work on them.
-
Replying to [comment:14 rene j.v. bertin]:
the whims of someone who is apparently on a personal crusade against everything MacPorts related.
This is very low, considering that you claimed we insulted you in the first place.
Noone has a personal crusade against MacPorts, notably seeing the time I spent today to answer you.
Your code must be cleaned to be merged. You don't like it, too bad, but don't claim it's because we do not like you.
-
No, you only told me what not to do (more precisely that it was wrong or bad, where you must have meant that that's your opinion about things). I've asked for specific guidelines exactly because I felt that otherwise I'd enter a trial-and-error process where I'd have to guess your ideas by trying until the "this is bad" stamps went away. No thank you, that's not how I work, and I don't see either how it's in your interest to spent more time telling me what (not) to do rather than doing it yourself.
Original author: rene j.v. bertin
-
Yes, I told you exactly what to do for the lua code, aka decommenting the code ifdef.
Moreover, I told you, in comment 8, to use the posix/dirs.c if you want to have Linux-like filesystem.
Finally, if you don't want this solution, on your core patch, I told you how to change it so you can get the minimum patch, and not a big patch, with incorrect coding style, so you can pass code review.
-
So ... against better knowledge, round N of our kampf ...
Replying to [comment:12 jb]:
The first part of the dir patch is totally wrong:
First off, you consider it wrong. Curiously, we only learn about that after I tried to upload a final set of patches that incorporate all suggestions made earlier, which was also after I clearly asked for guidance on a point or 2 that I wasn't sure about.
- comments are bad (DATE, seriously?)
The fact YOU don't like to put a date in a comment doesn't make it bad practice or a bad comment. Putting date tags in changes is extremely useful if you think you're potentially introducing issues that will not be discovered immediately, and you don't want to rely on metadata like from git to figure out when relevant code has been changed. Don't like it? That's fine, just ask me to remove it in a way that doesn't imply I'm a charlatan.
- changing style for no reason
Imposing style is bad and stupid. See? That's a personal opinion (not necessarily mine) voiced as a universal truth and not conductive to a constructive, adult exchange.
Style isn't consistent already and changed from the 2.1.5 version of dirs.c . I did my best to comply with style in that version of the file (despite abhorring the coding style in question), and then to adapt to the style in the current version. If you have nothing more constructive to say about what lines need retouching I'll leave that to someone who actually has the coding style in his or her fingers. Edit: clarification that this is not unwillingness to change style, only to do so without indication where I missed something in my changes, in a file that doesn't appear to have consisted style already.
- adding case functions for no reason
What are case functions? If you refer to strcasecmp etc. you didn't read the comment I added about why I introduced them. Standard OS X systems use a case-insensitive filesystem, which does its best to preserve case, but it can simply not guarantee that case will never change. Opening "vlc.app/cOntENts/macOS/VLC" will succeed just as well as "VLC.app/Contents/MacOS/VLC", so in code you cannot IMHO assume that the filename you get returned from a file system lookup is in the case you expect. I agree that this is a change that seems to have nothing to do with MacPorts but frankly I don't see the use of discussing it under a separate context/ticket. I'm not a VLC developer, I'm sharing patches that I consider useful. Please don't expect the same kind of rigour from users taking the effort of contributing than from regular project developers.
- Add MacOS, while this is a term that should get away, and we remove from the codebase.
What does this mean? Are you referring to the fact I introduce a comparison to MacOS/VLC instead of to "VLC"? How could you possibly want to ignore the fact that app bundle executables live in a directory called MacOS, something that is not likely to change AFAIK? It may not have occurred to you, but it's not impossible that the commandline vlc version (/opt/local/bin/vlc in the standard MacPorts case, /sw/bin/vlc for Fink, etc) could match that comparison while it would be inappropriate. Adding the app bundle parent directory is a simple and effective way to test if we're really being run from inside an app bundle (and invoked with a full path, which appears to be important).
I really hope that you don't consider it normal or acceptable that VLC will no longer function if a file or directory component has different case than what's usual, on an operating system where that is in itself perfectly acceptable? Because I certainly don't share that opinion.
- uses FALSE/TRUE, and ints for Boolean! And all this just to set a boolean to true.
Oh! Gee! Horror! All that! Oh! And the boolean is also set to false, sometimes even before setting it to true! This is C, right? Remind me again what the type used traditionally is, for booleans? It don't know what irks you more, the fact I used int (for lack of something better that I knew of) or TRUE/FALSE instead of 1/0 or another way to denote true or false, but again this remark gives a very strong impression that the person writing it isn't neutral w.r.t. the changes or their author. It's yet another example of how NOT to avoid heating of an exchange.
The second part, changes the data() place completely and is way too complex to just add a new path folder, that could be defined at compile time.
Changing the returned data location is intended because it so happens that the data in question is installed in that location. The actual computation of the location comes from the linux version of dirs.c . There is indeed a branch that returns the posix data location, which may be redundant. It was my first attempt, until I realised that I was dealing with a linux-style and not a pure posix installation, but I cannot exclude that the branch is completely useless. I am not confident that I could change the build system such that a less complicated change in the data function would suffice, without spending hours learning autoconf/automake. Time I don't have.
what to do for the lua code, aka decommenting the code ifdef.
I have not yet had the time to look at the function again to ensure that your proposed solution is indeed the correct one that handles all possible cases. From the time I spent stepping through it in lldb I am not confident it is.
what to do for the lua code, aka decommenting the code ifdef.
I wrote an answer to that, but it never got posted (Chrome seems to have an issue with the decorated submit button and I usually realise that only much later). Using posix/dirs.c is not an option unless it can be decided at runtime. Otherwise the standard path lookups won't work when starting VLC from the app bundle.
Again, it is my intention that any changes I make are transparent in the way that the returned values are not changed when running the app bundle version. Their main role is to allow 3rd party applications to use libvlc because VLC itself cannot be used outside its app bundle for reasons I haven't yet identified.
Original author: rene j.v. bertin
-
Replying to [comment:20 rene j.v. bertin]:
This is C, right? Remind me again what the type used traditionally is, for booleans?
bool. Like all the rest of the VLC codebase, like the specification by C99.
Once again, you show your ignorance, as we could already see in #13838 (closed).
but again this remark gives a very strong impression that the person writing it isn't neutral w.r.t. the changes or their author. It's yet another example of how NOT to avoid heating of an exchange.
I've been doing most of VLC reviews for a long time, and give the same kind of comments. There is nothing personal. Your code is not good enough to be merged.
-
Still there, jkoehne? :) If you are, feel free to use whatever of my changes you deem useful, in the way you consider appropriate. I've always considered my patches more of an example than something ready to be incorporated.
https://trac.videolan.org/vlc/ticket/13836?action=comment-diff&cnum=21&version=1 https://trac.videolan.org/vlc/ticket/13836?action=comment-diff&cnum=22&version=1
As predictable as expected. At least I can move on now.
Once again, you should your ignorance, as we could already see in #13838 (closed).
I should what?
Let's be very clear: I'm not a professional developer by training who has been taught about all different C flavours and I have never claimed so. I've just been coding C in professional contexts since 1989.
However I can claim that my mastery of English and realism is enough to express myself correctly and avoid most unintended cultural/language pitfalls in internet exchanges. No insult intended, but you cannot and should realise that.
Original author: rene j.v. bertin
-
Rene: I don't see why I should be of help to a guy who a) fails to spell my name b) makes racist comments about team members on public mailing-lists and fails to excuse himself for that once the heat is over and c) uses WWII vocabulary to underline ones stance in an argumentation with a German and a French of Jewish descent.
I'm sorry to see what appears to have happened to the MacPorts project, which used to produce a useful product and used to have a welcoming community. But that was a different time.
-
Replying to [comment:23 rene j.v. bertin]:
https://trac.videolan.org/vlc/ticket/13836?action=comment-diff&cnum=21&version=1 https://trac.videolan.org/vlc/ticket/13836?action=comment-diff&cnum=22&version=1
As predictable as expected. At least I can move on now.
Yes, I explained what I meant, because I knew you would twist them, if they were not crystal clear.
Let's be very clear: I'm not a professional developer by training who has been taught about all different C flavours and I have never claimed so. I've just been coding C in professional contexts since 1989.
And I'm the "12 year old kid" as you called me? How is that possible that I know better than a professional working since 1989?
-
You write:
Replying to [comment:23 rene j.v. bertin]:
As predictable as expected.
And then, you do: https://trac.videolan.org/vlc/ticket/13836?action=comment-diff&cnum=23&version=1
Ironic, right?
-
Replying to [comment:24 fkuehne]:
Rene: I don't see why I should be of help to a guy who a) fails to spell my name
Sorry for that. You're not spelling my name correctly either, I will presume just as unintentionally so.
b) makes racist comments about team members on public mailing-lists and fails to excuse himself for that once the heat is over and c) uses WWII vocabulary to underline ones stance in an argumentation with a German and a French of Jewish descent.
I'm not going to apologise for something I didn't do. I don't know where the WWII vocabulary comes from, maybe the bad taste use of a german word for fight? I'm indeed not going to apologise for that, but it wasn't to be insulting to any form of religion, nor to be insulting to you. I'm of French and Dutch descent, so am quite aware of the stereotypes of both cultures, and no matter what you think, suggesting that someone sadly corresponds to such stereotypes is not racism. It's not even a true form discrimination, IMHO. It can be used as a subtle form of insult that I think perfectly acceptable under free speech guidelines, which is why I publicly admitted being sorry that it was seen by the target. I cannot however apologise for the fact that person visited a public archive to read things he finds insulting and objectionable.
I'm sorry to see what appears to have happened to the MacPorts project, which used to produce a useful product and used to have a welcoming community. But that was a different time.
This hasn't had to do with MacPorts for a long time, and it certainly isn't MacPorts who isn't the welcoming community. MacPorts is considerably more open to contributions, and imposes much laxer rules on how things are done.
Original author: rene j.v. bertin
-
I have a proposition, for the sake of productivity and the MacPorts and KDE/Mac communities.
My personal main interest is not so much to provide a full VLC app through MacPorts as it is to have libvlc in there, specifically to be able to have a functional phonon-backend-vlc. I see there's page about LibVLC . The build instructions that it contains seem to boil down to (if I haven't overlooked anything) 1) build VLC and 2) extract the headers and the libvlc.so binaries. I presume that includes the libvlccore.* binaries, and am of course referring to the standard build, not VLCKit.
I have some questions I would like to find an answer to but other priorities to pursue right now, so I'll fire away and see.
-
Is there a reason to prefer the VLCKit version, or rather the more Posix-compliant libvlc version? A framework is just a form of packing library, headers and related stuff in a single bundle, but it would be overkill if that would include libav* and all the other dependencies used by the different plugins. Also, VLCKit might not include CMake and pkg-config files, or does it?
-
Supposing libvlc would be the preferred choice, is there a way to tell bootstrap and/or configure to build things that way, to limit patching to the strict minimum? Will it be enough to use
--disable-macosx-vlc-app, or is that going to interfere with the options to activate building of Mac-specific audio and/or video plugins? -
Regarding
VLC_PLUGIN_PATH: I hope there are standard ways to avoid having to use that variable to point client apps tovlc-src/modules?
Original author: rene j.v. bertin
-
added Platform::macOS label
-
Well, since noone seems to care enough, except a very insulting developer, who provided code of very dubious quality, I will close this.
If in the future, someone with half-a-clue cares about it, patches welcome. But reopen a new ticket.
This thread will be locked and any comment under this one will be deleted.
added Status::will not fix label