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

Don't depend on any binary blobs #4267

Open
kuruczgy opened this issue Jan 11, 2025 · 7 comments
Open

Don't depend on any binary blobs #4267

kuruczgy opened this issue Jan 11, 2025 · 7 comments

Comments

@kuruczgy
Copy link

Currently the tests depend a bunch of prebuilt binaries through submodules, e.g. External/fex-gcc-target-tests-bins and some others.

Binary blobs are bad from an auditability perspective. One could say that "those are just for tests so they don't count", but I am not aware of any distro packaging system that lets one (easily) selectively clone submodules, so in practice they are all present during builds.

Building the test binaries from source would be an obvious idea, but e.g. pulling in the whole gcc repo is probably even less desirable than the current solution, so I see why it's not done.

Ideas for alternatives:

  • Instead of using submodules, manually git clone --depth 1 the bin repos in the CI.
  • Maybe there is some way to configure .gitmodules to not clone these submodules by default with git clone --recursive? E.g. not sure if submodule.<name>.fetchRecurseSubmodules does that.

In either case, it should be documented which submodules are needed for building FEX and which ones only for the tests (and which tests), currently it's a bit of a guessing game.

Also honorable mention to Source/Windows/wine_builtin.bin, but that's small enough to be audited by hand with a hex editor, so it's not a priority. (It's just text with some null bytes afterwards.)

Sonicadvance1 added a commit to Sonicadvance1/FEX that referenced this issue Jan 12, 2025
Instead generate it with an echo command which is easier to see what it
is doing. It's a trivial file that is just a string and 16 zeroes.

Brought up by FEX-Emu#4267
@Sonicadvance1
Copy link
Member

Our previous Arch package file would skip the submodule checkout for the test binaries, but that's just an adjacent topic.

As you thought, building these on the CI systems aren't a viable solution, especially as some of them are a pain to compile, take a long time to compile, and/or compiled with -march=native on some AVX supporting system. Additionally they would need cross compiling on any ARM CI machine which adds its own layer of pain. This is why we chose to have them as just binary submodules.

For our Ubuntu PPA, we also just remove a bunch of folders that are unnecessary for reducing the size of the source package.

rm -Rf $TEMP_SOURCE/External/{fex-gcc-target-tests-bins,fex-gvisor-tests-bins,fex-posixtest-bins}
rm -Rf $TEMP_SOURCE/.git
rm -Rf $TEMP_SOURCE/unittests
rm -Rf $TEMP_SOURCE/External/vixl/src/aarch32/
rm -Rf $TEMP_SOURCE/External/vixl/test

So it can be seen that basically just the bins folders are unnecessary, the other folders are just tertiary things that don't really matter. There can probably be some documentation somewhere about these three folders being unnecessary.

@kuruczgy
Copy link
Author

Thanks, that list is useful, I agree that it would be useful to document them somewhere properly.

Regarding unit tests, does the build system automatically detect if the bins submodules are missing and only run the rest of the tests, or is some special flag needed? (I think for a "proper" distro package it would not hurt to run at least some of the unit tests.)

@Sonicadvance1
Copy link
Member

The build system doesn't automatically run the tests. It only runs when manually invoked, which our github CI scripts do. ninja tests runs all of them, but the github CI system runs the sub-tests individually.

@kuruczgy
Copy link
Author

I see. What would you recommend for a distro package? (I am thinking some reasonable subset that can basically act as a sanity check that something did not go horribly wrong with the distro build process, and does not take too many commands to run (would prefer to not have to specify all the sub-tests individually.))

@Sonicadvance1
Copy link
Member

The asm tests are a good sanity test, usually takes less than a minute on our 8-16 core ARM CI machines. Not sure if package maintainers care beyond that.
As for build options, FEX's cmake will automatically tune to -march=native so will need some options to not be built for whatever ARM version the server is running.
An example can be seen for cmake options in our ppa rules https://github.com/FEX-Emu/FEX-ppa/blob/main/deb_base/rules#L23-L39

In particular @TUNE_CPU@ and @TUNE_ARCH@ should both be generic for supporting an ARMv8.0-a binary (Although we may raise our minspec to v8.2+ in the future. #4120)

No idea how anything other than debian based repos are supposed to build our cross-compiling thunks for the Linux binaries atm, since it needs a cross compiler and some x86 rootfs.,

For the wine integration packages, it also currently needs a downstream mingw-clang toolchain to work around bugs in their arm64-windows targets.
As seen in our ppa, I just tar up the whole toolchain in the source package to work around it. https://github.com/FEX-Emu/FEX-ppa/blob/main/deb_wine_base/rules#L59-L65

As someone that isn't a package maintainer, I don't really know what are blockers for real maintainers to make packages of FEX, I just play in my little sandbox and hope for the best.

@kuruczgy
Copy link
Author

Thanks. Might still ask a few more packaging related questions in the future as they come up, if you don't mind.

Regarding build docs, it seems like currently it's all on the wiki, so I cannot submit a PR for that.

For the initial packaging I think I will ignore the thunks and wine integration. (Though getting a cross compiler for NixOS at least should not be too difficult, so should definitely be possible to add thunk support in the future.)

Thanks for the heads up about the ARM minspec raise, I might want to try to already package for that to be future proof.

@alyssarosenzweig
Copy link
Collaborator

fwiw, the fedora spec might be useful to you https://src.fedoraproject.org/rpms/fex-emu/blob/rawhide/f/fex-emu.spec

submodules are manually cloned there.

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

No branches or pull requests

3 participants