blueprint-freespeech / ricochet-build

Repo for building ricochet-refresh
https://github.com/blueprint-freespeech/ricochet-refresh
BSD 3-Clause "New" or "Revised" License
3 stars 4 forks source link

Ricochet Refresh for linux-aarch64 #19

Open NoisyCoil opened 6 months ago

NoisyCoil commented 6 months ago

This PR adds support for linux-aarch64 to Ricochet Refresh. The gcc cross-toolchain used to build from x86_64 is adapted from tor-browser-build. This PR is marked as a draft since:

Note: the patchset is applied to the last released tag, not to current main.

morganava commented 6 months ago

This looks like a great start!

Looks like it may be time to actually start an alpha channel for this to live in :eyes:

NoisyCoil commented 6 months ago
  • please reword the individual commits in the format: ${project-name}: thing the patch does
  • prefer adding support for aarch64 in dependency order (ie swap locations of Qt and zlib commits in the history)

Done.

  • do we need both linux_cross and linux-cross
  • is it necessary to make the dpkg --add-architecture line conditional? rather, shouldn't it just be dependent on whether var/arch_debian is defined?

To summarize, linux-cross is the variable associated to target linux-cross (hence the dash instead of the underscore), which in turn is the target for linux architectures which are actually cross-compiled from x86_64. i686 doesn't qualify as one, as it doesn't need a true cross-compiler with its sysroot (i686 is basically just 32-bit compilation on a 64-bit system). linux_cross, on the other hand, AFAICS, is only used to tell container-image to --add-architecture, and is only defined for i686. So those two variables do very different things despite their names.

dpkg --add-architecture cannot be made dependent on whether arch_debian is defined, because the latter is a target variable and some targets (namely, the cross-compiled targets) need containers (namely, those which build host tools) with no dpkg --add-architecture added to avoid duplicate project builds.

Still, it bothers me too that there are two similar variables. To avoid this we could plainly remove linux_cross (note the underscore) and no_crosscompile (currently only used by linux-cross, note the dash), define a new add_architecture variable which is set to 1 in both the linux-i686 and linux-cross targets, and make --add-architecture conditional on that alone instead, i.e. not even on linux-cross:

  [% IF c("var/add_architecture") %]
    # Add cross-compilation dpkg architecture
    dpkg --add-architecture [% c("var/arch_debian") %]
  [% END -%]

no_crosscompile can be replaced either by add_architecture: 0 per-project or by suitable target_replaces, something I didn't do in tor-browser-build because there are far more projects there, and the target replacements would clog up the configuration files. For ricochet-build it's fine I believe.

  • is it possible to just always build gcc as a cross-compiler? Overkill for x86 and x86_64 but maybe better somehow in terms of maintenance when it comes time to do compiler upgrades.

Not sure it would improve maintainability:

  1. bootstrapping gcc/building a sysroot (which is what I assume you mean by "always building gcc as a cross-compiler", since otherwise gcc for some arch is just gcc for some arch) comes with its own issues which are often arch-specific, see e.g. the aarch64 glibc patches I had to backport to build the aarch64 sysroot in tor-browser-build
  2. during bootstrap, the build process may get confused on which compiler it's using (we have two gcc compilers for the same architecture on PATH!) and this introduces setup differences between compiling gcc-cross for a foreign architecture and compiling gcc-cross for the same architecture as the hosts. For example, I just checked and a build script of mine (similar to but not the same as the one which is currently in tor-browser-build and in this MR) fails in building an x86_64 sysroot, while it succeeds in building a ton of other archs, spanning from aarch64, to mips, to riscv to powerpc
  3. using the sysroot itself is not trivial, especially if you want to use it together with system dev packages. It already is a nightmare (see qt) to make the sysroot work with foreign-architecture dev packages, I expect this to be much worse for the host architecture
  • it will be some time before upstream supports aarch64 tor-expert-bundle, can you make the code-gen in ricochet-refresh and inclusion of the PTs in package conditional on !linux-aarch64
  • can you also open an MR on ricochet-refresh to disable the bridge entry widget in the tor config dialog if there are no built-in pluggable transports defined?

Will look into these.

NoisyCoil commented 5 months ago
  • can you also open an MR on ricochet-refresh to disable the bridge entry widget in the tor config dialog if there are no built-in pluggable transports defined?

Done.

  • it will be some time before upstream supports aarch64 tor-expert-bundle, can you make the code-gen in ricochet-refresh and inclusion of the PTs in package conditional on !linux-aarch64

I submitted a PR for making inclusion of pluggable transports conditional, then rebased this MR onto that one and deactivated pluggable transports for linux-aarch64 (so it's not importing mine any more).

morganava commented 5 months ago
  • can you also open an MR on ricochet-refresh to disable the bridge entry widget in the tor config dialog if there are no built-in pluggable transports defined?

Done.

Thanks, merged it!

morganava commented 5 months ago
  • do we need both linux_cross and linux-cross
  • is it necessary to make the dpkg --add-architecture line conditional? rather, shouldn't it just be dependent on whether var/arch_debian is defined?

To summarize, linux-cross is the variable associated to target linux-cross (hence the dash instead of the underscore), which in turn is the target for linux architectures which are actually cross-compiled from x86_64. i686 doesn't qualify as one, as it doesn't need a true cross-compiler with its sysroot (i686 is basically just 32-bit compilation on a 64-bit system). linux_cross, on the other hand, AFAICS, is only used to tell container-image to --add-architecture, and is only defined for i686. So those two variables do very different things despite their names.

dpkg --add-architecture cannot be made dependent on whether arch_debian is defined, because the latter is a target variable and some targets (namely, the cross-compiled targets) need containers (namely, those which build host tools) with no dpkg --add-architecture added to avoid duplicate project builds.

Still, it bothers me too that there are two similar variables. To avoid this we could plainly remove linux_cross (note the underscore) and no_crosscompile (currently only used by linux-cross, note the dash), define a new add_architecture variable which is set to 1 in both the linux-i686 and linux-cross targets, and make --add-architecture conditional on that alone instead, i.e. not even on linux-cross:

  [% IF c("var/add_architecture") %]
    # Add cross-compilation dpkg architecture
    dpkg --add-architecture [% c("var/arch_debian") %]
  [% END -%]

no_crosscompile can be replaced either by add_architecture: 0 per-project or by suitable target_replaces, something I didn't do in tor-browser-build because there are far more projects there, and the target replacements would clog up the configuration files. For ricochet-build it's fine I believe.

Ok this all sounds good to me. This project does diverge from tor-browser-build a fair bit because it was built up from nothing after having a better understanding of how upstream works and could be improved when you're starting fresh.

morganava commented 5 months ago

Another nit, can you also ensure new files end in a newline?

NoisyCoil commented 5 months ago

Ok this all sounds good to me.

It turns out that setting linux_cross: 0 is useless ATM in main, at least AFAICS. linux-i686 sets linux_cross: 1 for all targets in order to --add-architecture in containers. linux_cross: 0, on the other hand, is set only as a project variable in cmake-build. But cmake-build is only built by cmake with a target_replace to linux-x86_64, so linux_cross: 0 is not actually needed. Also, AFAICS, there's a bug in how linux_cross is used in container-image. It should be

[% IF pc(c('origin_project'), 'var/linux_cross', { step => c('origin_step') }) %]

rather than

[% IF c("var/linux_cross") %]

meaning that linux_cross should be set within its origin project, not within container-image itself (to be able to deactivate --add-architecture when needed). The fact that builds do work (in the sense that cmake-build is built as if linux_cross: 0, despite the bug) demonstrates what I was saying above I believe.

Thus to avoid the double linux_cross/linux-cross in this MR I would first get rid of the places where var/linux_cross is used for no real reason, then solve the aforementioned bug and finally rename var/linux_cross to var/add_architecture, which better explains what that variable does. Then in this MR we can set var/add_architecture: 1 as part of the definition of the linux-cross target, and keep var/linux-cross as the build variable associated to the linux-cross target. I have a MR ready for changing linux_cross->add_architecture. What do you think?

morganava commented 3 months ago

What do you think?

This all looks reasonable to me. Sorry for the slight (lol) delay!