Skip to content

Conversation

@holdengreen
Copy link
Contributor

@holdengreen holdengreen commented Apr 28, 2021

Hi I've finally got something!

I've got ffmpeg 4.4 to run in SerenityOS.

It is now using a sha256 hash that I derived because I was having issues with gpg.

I have not extensively tested it in serenity yet, but it does seem to be missing media/library codecs that would make it more practically useful.

I'm still able to continue improving this port.

port=ffmpeg
version=4.4
useconfigure=true
files="https://ffmpeg.org/releases/ffmpeg-4.4.tar.gz ffmpeg-4.4.tar.gz a08992a9f62cf9877582565544e8e799"
Copy link
Member

Choose a reason for hiding this comment

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

You can interpolate with ${version} here twice in the filenames

version=4.4
useconfigure=true
files="https://ffmpeg.org/releases/ffmpeg-4.4.tar.gz ffmpeg-4.4.tar.gz a08992a9f62cf9877582565544e8e799"
auth_type="md5"
Copy link
Member

Choose a reason for hiding this comment

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

Please use the sha256 auth type

Copy link
Member

@linusg linusg left a comment

Choose a reason for hiding this comment

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

please split the first commit into two separate, atomic commits :)

port=ffmpeg
version=4.4
useconfigure=true
files="https://ffmpeg.org/releases/ffmpeg-4.4.tar.gz ffmpeg-4.4.tar.gz a08992a9f62cf9877582565544e8e799"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
files="https://ffmpeg.org/releases/ffmpeg-4.4.tar.gz ffmpeg-4.4.tar.gz a08992a9f62cf9877582565544e8e799"
files="https://ffmpeg.org/releases/ffmpeg-${version}.tar.gz ffmpeg-${version}.tar.gz a08992a9f62cf9877582565544e8e799"

version=4.4
useconfigure=true
files="https://ffmpeg.org/releases/ffmpeg-4.4.tar.gz ffmpeg-4.4.tar.gz a08992a9f62cf9877582565544e8e799"
auth_type="md5"
Copy link
Member

Choose a reason for hiding this comment

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

auth_type=md5 is not supported anymore, please use sha256

Comment on lines 13 to 17
--arch="x86_32" \
--cc="i686-pc-serenity-gcc -std=gnu99" \
--cxx="i686-pc-serenity-g++ -std=gnu99" \
--disable-network \
--enable-cross-compile
Copy link
Member

Choose a reason for hiding this comment

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

  • Please don't mix tabs and spaces (spaces only)
  • Please don't hardcode i686 (use ${CC} and ${CXX} instead of i686-pc-serenity-gcc and i686-pc-serenity-g++

@linusg
Copy link
Member

linusg commented Apr 28, 2021

Also, the commit message should say "Ports", not "Port".

@holdengreen holdengreen changed the title Port: ffmpeg 4.4 (+ relevant LibC additions) Ports: ffmpeg 4.4 (+ relevant LibC additions) Apr 28, 2021
@holdengreen
Copy link
Contributor Author

Thanks guys! I think I've covered it...

@holdengreen holdengreen requested a review from linusg April 29, 2021 00:32
useconfigure=true
files="https://ffmpeg.org/releases/ffmpeg-${version}.tar.gz ffmpeg-${version}.tar.gz a4abede145de22eaf233baa1726e38e137f5698d9edd61b5763cd02b883f3c7c"
auth_type="sha256"
makeopts="-j$(nproc)"
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems gratuitous, I don't think a random port should force parallelism on the build system.

Copy link
Contributor Author

@holdengreen holdengreen Apr 29, 2021

Choose a reason for hiding this comment

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

Good call. I got that line from the lua package script. (which is what I originally copied and modified because I'm familiar with lua)

During testing I compiled ffmpeg without parallelism (to make debugging easier) and I will warn that it did take a non-negligible amount of time to compile...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I took care of it!

@awesomekling awesomekling merged commit 0d74dff into SerenityOS:master Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants