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

Build system changes for BSD #5217

Merged
merged 19 commits into from Jul 20, 2023
Merged

Build system changes for BSD #5217

merged 19 commits into from Jul 20, 2023

Conversation

robxnano
Copy link
Contributor

@robxnano robxnano commented Jun 26, 2023

These changes are mostly to fix building on the different BSD variants, plus some changes to the build scripts to remove the bash requirement. As BSD and macOS don't have bash installed by default, it means one less program to install. I also added some colors to the configure script, which aren't strictly necessary but make the output a little easier to read. This change can be removed if you'd prefer to keep it as it is.

Tested on:

  • Windows 10+ (via MinGW)
  • macOS 10.13+
  • Ubuntu Linux
  • FreeBSD
  • NetBSD
  • OpenBSD

@bradleysepos bradleysepos self-assigned this Jun 26, 2023
@sr55 sr55 requested review from galad87, bradleysepos and a user June 26, 2023 19:44
@sr55 sr55 added this to the 1.7.0 milestone Jun 26, 2023
@galad87
Copy link
Contributor

galad87 commented Jun 27, 2023

I think @brad0 worked a bit on BSD compatibility too, they might want to take a look too.

@galad87
Copy link
Contributor

galad87 commented Jun 28, 2023

Everything looks fine on macOS too.

@sr55
Copy link
Contributor

sr55 commented Jul 2, 2023

LGTM as well. Thanks

Copy link
Contributor

@bradleysepos bradleysepos left a comment

Choose a reason for hiding this comment

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

Overall looks great, just a few comments. Thanks for doing this.

make/configure.py Outdated Show resolved Hide resolved
make/configure.py Show resolved Hide resolved
libhb/handbrake/ports.h Show resolved Hide resolved
configure Outdated Show resolved Hide resolved
@brad0
Copy link
Contributor

brad0 commented Jul 2, 2023

Although most looks Ok, a lot has little or nothing directly to do with *BSD. Like the changes for BeOS, POSIX threads, Python, POSIX compliancy for some scripts, etc. Typically I would split those up into smaller PRs.

Rewrote repo-info.sh, tag-release.sh and build-presets.sh to remove
the need for bash when compiling.
- Update python versions
- Add libjpeg-turbo to modules to avoid conflict
- Fix failure to make metainfo file
- The control characters for the colored text will only be used if
the output is being sent to a terminal
- Error messages have been consolidated into a single function
- Don't turn arguments of the form --arg=value into exports
Needed to detect failures in CI.
There needs to be only one space before the build number.
Otherwise the shared library gets built without a version number.
It can be enabled on other operating systems with --enable-gtk
- Supported on Linux, FreeBSD, OpenBSD, NetBSD, and MinGW
- Simplified support checks in configure.py
@bradleysepos
Copy link
Contributor

Looks good now. Approved.

Although most looks Ok, a lot has little or nothing directly to do with *BSD. Like the changes for BeOS, POSIX threads, Python, POSIX compliancy for some scripts, etc. Typically I would split those up into smaller PRs.

I have to agree with @brad0 here (Brads united).

This is a fairly large set of changes that I would prefer to be split into multiple PRs, but I also do not wish to stifle positive development this instance. Let's merge this and aim for smaller chunks in the future. Thanks @robxnano for the good work!

@galad87 galad87 merged commit bfc31f0 into HandBrake:master Jul 20, 2023
5 checks passed
@robxnano robxnano deleted the bsd-build branch July 20, 2023 12:49
@brad0
Copy link
Contributor

brad0 commented Jul 22, 2023

@robxnano Thank you for the work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants