search

LMS-Community / slimserver-vendor

Third-party software used with Lyrion Music Server
https://lyrion.org
42 stars 68 forks source link

Add Travis-CI support, upgrade libmediascan & refactor buildme #65

Closed fsbruva closed 5 years ago

fsbruva commented 5 years ago

This PR is a squashed merge of 155 commits, and represents about a year of effort by @fsbruva & @chincheta0815. The gory details of the individual changes and efforts that went into this are documented below, in the respective devel/7.9 and upgraded_libmediascan branches, as well as the respective forks of the libmediascan repository.

Add Travis-CI support Now builds (and PRs!) can be automatically tested across a wide range of Perl, using both gcc and clang on Ubuntu. Additionally, builds can be run on macOS using the stock Perl and wide array of XCode versions. This required refactoring all tests using which or checked the value of $? to be safer. Now, all command checks and header finds are tested in a subshell or in the if test. This is because we need the Travis builds to fail when a single subcommand (such as build ffmpeg) fails, so we must run the script with the -o errorexit flag. Consequently, tests like the search for a duplicate libungif would cause the entire build to fail when the desired outcome (pre-existing libungif wasn't found) occured.

Update libmediascan and dependencies libmediascan: Update all API hooks in libmediascan and libdlna source code for new ffmpeg Add correct symbolic linking flags to Perl Makefile to enable ASM for ffmpeg Add option to omit intl library, as macOS and FreeBSD don't have GNU gettext Incorporate FreeBSD and SunOS source code patches as precompiler directives Build Sub-Uplevel, Tree-DAG_Node, and Test-Warn to avoid prereq warnings Update Darwin hints to add support for macOS 10.10+ Set osx-min-version to 10.9 due to Carbon framework deprecation in 10.8 Update macOS version determination method in Darwin hints

db: Upgrade from 5.1.25 to 6.2.32 Remove unneeded src_dbinc_atomic patch (upstream already patched)

giflib: Upgrade from 4.1.6 to 5.1.4 Add version check in buildme.sh to force rebuild as needed, due to API changes

libjpeg-turbo: Upgrade 1.1.1 to 1.5.3 Build universal ppc/i386 binaries only for macOS 10.5 Build universal i386/x64 binaries only for macOS below 10.7 Build only x64 binaries for macOS 10.7+, since 10.7 and up are 64-bit only Use patch to disable unneeded features (instead of using custom jmorecfg.h file) Note: Upgrade beyond 1.5.3 will require CMake as new dependency

libpng: Upgrade 1.4.3 to 1.6.36 Use built-in pngusr.dfa methods to disable unneeded features

ffmpeg: Upgrade 0.8.4 to 4.1.1 Elevate nasm as sole assembler dependency, due to ffmpeg no longer permitting yasm Add version check to force rebuild as needed, due to API changes Remove padre patch, because --arch=sparc flag provides required functionality Explicitly disable iconv to avoid build errors on macOS Convert all =~ OS tests from glob to proper regex Build universal ppc/i386 binaries only for macOS 10.5 Build universal i386/x64 binaries only for macOS below 10.7 Build only x64 binaries for macOS 10.7+, since 10.7 and up are 64-bit only

Changes to buildme.sh Add explicit descriptions of CLI arguments Add job count variable and CLI knob to pass through to make (parallel build) Remove perlbrew info, build hints and OS/Perl combo information (move to README) Assume Perl is in PATH, or that -p points to valid Perl Remove Perl version hunt, which will cause boundless growth of script Aggregate all OS specific config options within a single case statement Perform Perl version/flags detection earlier to avoid SunOS build errors Modernize macOS version detection method to use sw_version Convert macOS version to number for easier tests Set all macOS versions greater than 10.9 to use 10.9 as min-version flags, due to the Carbon framework getting deprecated in 10.8. Establish new FLAGS_COMMON variables to encompass all OS specific flags Convert all =~ compiler tests from glob to proper regex Implement Apple's rebrand of OSX to macOS

Modules and other miscellany README.md: Add build notes for OmniOS and Red Hat flavors of Linux Add all perlbrew notes, build hints and known OS/Perl combinations

Audio-Scan: Remove 0.93 and 0.95, all targets can build Audio-Scan-1.02

IO-Socket-SSL: Upgrade 2.052 to 2.060

Net-SSLeay: Upgrade 1.82 to 1.85 Add OpenSSL patch to enable build on Solaris

Test-Simple: Add version 1.3.02141 and build for Perl 5.8, because latest DBD-DBI has bug with prereq version of Test::More (needs method that shipped with 5.10).

darwin.pl: Add support for macOS 10.10+ Set osx-min-version to 10.9 due to framework deprecations in 10.8

This commit built successfully on Travis-CI for:

This commit built successfully on test machines running:

mherger commented 5 years ago

I'm sorry, there's no way I'm going to merge this as is. Please split it up in to manageable chunks. Describe with every request what problem you're solving etc. I only see one reported issue - and that's not fixed by this change. So what are you fixing?

Eg. all the module updates might look like easy changes for a single platform. But when we have to build rpm or deb packages which would come with a mix of various versions, this will be a major headache. Therefore I'm very reluctant to update modules unless there's a very good reason.

chincheta0815 commented 5 years ago

I totally understand what you mean.

See it as an effort to make LMS a little bit fit for the future. There are some version changes like ffmpeg 0.8 to 4.1 that are tremendous. Another thing is that to my knowledge ActiveState Perl 5.14 is hard to find in the days of Perl 5.22 maybe a change to StrawberryPerl with travis-ci support can help here to help other develop on Windows...

The idea of using travis-ci for me sounds really awesome as a lot of tests run very smooth. Maybe the packaging can also be incorporated in a certain stage avoiding headaches. I actually do not understand how all the packaging works. To my knowledge the most parts are statically linked provoking as few dependencies as possible. Maybe automated tests can help?

Now concerning the changes: In libmediascan and buildme all the patches were removed, incorporating them into the code. This led to a less complicated buildme.sh. The results helps a lot when adding new platforms since the debugging now is easier.

Having in mind that ffmpeg is really old the latest version also needed some updates in its dependencies (libjpeg, etc.). Adding the patches into libmediascan led to less patches, removing the patch lines in buildme.sh. It was then just adapted to the latest ffmpeg.

IO::Socket::SSL is not automatically built and needs NetSSLeay which deprecated some old openssl functions. Version 1.85 of NetSSLeay already has a patch for that, upstream is informed.

fsbruva commented 5 years ago

I feel like there might be a few things going on.

Is it that the commit is too big? Does upgrading the bundled Perl modules pose a problem? Must all PRs solve problems? If so, what is the threshold for a problem, and must it be a documented issue within this repository first? Do security vulnerabilities from really old software count as problems? Pr #58 updated source code for the sake of updating. What about code readability and usability? PRs #49 and #50 make it seem like code tidy-ups "solve problems." I am happy to break this up into smaller chunks. Just trying to gauge ahead of time which PRs to focus on, and how to present them.

mherger commented 5 years ago

I feel like there might be a few things going on.

Correct.

Is it that the commit is too big?

Yes, it's too big. That's my main concern. There's no way I can understand what is going on in there. What is required to solve which challenge. What is cosmetics. What is breaking backwards compatibility, platform support etc. Unfortunately we've seen such issues with larger re-factoring before. You might remember some it.

Does upgrading the bundled Perl modules pose a problem?

As outlined in my initial response: yes, they can pose a problem. Look at the mess we already have supporting different versions of Audio::Scan.

Must all PRs solve problems?

IMHO yes. This is not a piece of art. I wouldn't even call it a project. But it's a tool we need to build binaries to run LMS. If that does not work, then there is a problem we can solve. You could even argue that readability of the code could be a problem. But tbh: as long as that thing works, I don't care. And when it doesn't work, then I try to improve things while fixing the issue. But updating modules to improve the readability of the script is the wrong motivation.

The problem with this tool is that I need it every few months, probably once a year. And most of the time it just works. Which is also the reason why I am not familiar enough with it to be able to seriously review the changes you submitted in one large PR.

If so, what is the threshold for a problem, and must it be a documented issue within this repository first?

That's a fair point. At least the pull request should be clear about what challenge it's solving. Travis support, I'm sorry to say, I don't consider one. As I said above this is a tool I use once a year. I don't see the need to run it regularly. I've got bigger fish to fry.

Do security vulnerabilities from really old software count as problems?

They would be good candidates, yes. But would you be in a position to update all supported platforms we have? Otherwise it's a moot point. Yes we can use more up to date modules going forward when adding support for new platforms. But then again this can be split into manageable pieces, not as part of a huge PR which does cover several other aspects, too.

Pr #58 updated source code for the sake of updating.

The discussion for this PR happened outside github. The new version has massive performance improvements on slower platforms.

What about code readability and usability? PRs #49 and #50 make it seem like code tidy-ups "solve problems."

You're right. But these were small changes. Nothing compared to the 2-3k changed lines of code of this PR...

I am happy to break this up into smaller chunks. Just trying to gauge ahead of time which PRs to focus on, and how to present them.

That sounds great. Alas I have to be clear here: I don't have much time and resources to spend on this. While I salute the energy and effort you put in to this, I'd prefer to see such effort be invested in the LMS code itself, rather than a tool we need to run once or twice a year...

Unfortunately the past has shown that I sometimes did merge PRs I should not have accepted. Stuff got broken more than once. IO::Socket::SSL does not belong here etc. I accepted them because I didn't want to look defensive. But I quite obviously did not my duty re-viewing and understanding what I was merging well enough. I don't want to do this again.

Now the good thing is we are on github, this is open source. Feel free to provide your branch to the world. This additional exposure and testing might convince me to merge it or parts of it at some point. I just don't want to break my tool unless I see a very good reason to do so.

chincheta0815 commented 5 years ago

@mherger As you pointed out that you use that script once or twice a year and do not have that much time: Would it help if travis-ci more or less also tests the creation of the packages and maybe does package them for the platform that are mentioned in slimserver-platforms? If so: Is there a README on how to use these? Maybe we could help. I am playing right now a little bit with Windows support for travis-ci...

fsbruva commented 5 years ago

Understood on all points. Thanks for the response. Honestly, I am only good at shell scripting, and have zero Perl experience. And my brother is the one with a Squeezebox, so I am not really able to test much of anything.

By way of explanation, I pursued the travis support precisely because I knew it was difficult for you to test proposed changes, and only cared about "it must work." With travis, any PR submitted would have the build tested to make sure it worked. Automatically testing proper functionality might be of higher performance, but since so much is the gui, that will probably not be doable with Travis. However, since I don't have any dedicated linux machines and no way to test macOS, travis is a crucial way for me to test code before I offer it to you.