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

fix: cross-compiling and don't treat x86 as the default case #295

Merged
merged 2 commits into from
Oct 4, 2024

Conversation

chewi
Copy link
Contributor

@chewi chewi commented Feb 25, 2024

Description

If you set arch-specific CFLAGS, then ffmpeg's configure script may fail when it tries to use these flags against the build host's compiler.

Also use CMAKE_SYSTEM_PROCESSOR to set up cross-compiling without relying on any custom variables. ffmpeg normalises its --arch option and will accept just about any string that you'll likely throw at it.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Dependency update (updates to dependencies)
  • Documentation update (changes to documentation)
  • Repository update (changes to repository files, e.g. .github/...)

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

Branch Updates

LizardByte requires that branches be up-to-date before merging. This means that after any PR is merged, this branch
must be updated before it can be merged. You must also
Allow edits from maintainers.

  • I want maintainers to keep my branch updated

@chewi
Copy link
Contributor Author

chewi commented Feb 25, 2024

None of the failures were caused by this change.

@ReenigneArcher
Copy link
Member

ReenigneArcher commented Feb 29, 2024

ppc64el build was fixed if you rebase

@chewi
Copy link
Contributor Author

chewi commented Mar 3, 2024

My bad, leftover bracket.

@chewi
Copy link
Contributor Author

chewi commented Mar 3, 2024

That's weird. CMake is supposed to respect CC/CXX. I don't think it even executed those export lines though.

@ReenigneArcher
Copy link
Member

Sorry for the delay, I'm trying to catch up with things.

So changing the compiler for macOS... will we need to make sure the compiler used for Sunshine is the same one as used here?

@chewi
Copy link
Contributor Author

chewi commented Mar 31, 2024

So changing the compiler for macOS... will we need to make sure the compiler used for Sunshine is the same one as used here?

My change originally switched it from GCC to Clang, as that is what it was supposed to use to match the rest of the build. However, that didn't work, so I adjusted my change to keep it using GCC, but more explicitly than before. The fact that it doesn't match the rest of the build evidently wasn't a problem before, and I wouldn't expect it to be anyway. It is quite common to mix GCC and Clang binaries.

@LizardByte-bot
Copy link
Member

It looks like this PR has been idle for 90 days. If it's still something you're working on or would like to pursue, please leave a comment or update your branch. Otherwise, we'll be closing this PR in 10 days to reduce our backlog. Thanks!

@chewi
Copy link
Contributor Author

chewi commented Jun 30, 2024

Rebased. I'd still like to see this merged.

@LizardByte-bot
Copy link
Member

It looks like this PR has been idle for 90 days. If it's still something you're working on or would like to pursue, please leave a comment or update your branch. Otherwise, we'll be closing this PR in 10 days to reduce our backlog. Thanks!

@ReenigneArcher
Copy link
Member

Again, sorry for the delay.

I'm a bit hesitant to merge this as I think the right approach for this repo is to convert it into a full cmake project, so it can be easily built as a standalone library or inside of Sunshine.

If you set arch-specific CFLAGS, then ffmpeg's configure script may
fail when it tries to use these flags against the build host's compiler.

Also use CMAKE_SYSTEM_PROCESSOR to set up cross-compiling without
relying on any custom variables. ffmpeg normalises its --arch option and
will accept just about any string that you'll likely throw at it.
We want ffmpeg to respect the compiler chosen by CMake, but that
defaults to Xcode/Clang, where GCC was used for this case previously.

Simplify the logic surrounding this change by always setting
CMAKE_TOOLCHAIN_FILE in the pipeline. It does nothing when set to a
empty value.
@chewi
Copy link
Contributor Author

chewi commented Oct 4, 2024

That would probably be a good thing for me.

@ReenigneArcher ReenigneArcher changed the title Fix cross-compiling and don't treat x86 as the default case fix: cross-compiling and don't treat x86 as the default case Oct 4, 2024
@ReenigneArcher ReenigneArcher merged commit 303564f into LizardByte:master Oct 4, 2024
12 checks passed
@chewi chewi deleted the cross-compiling branch October 4, 2024 22:39
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.

3 participants