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

Add support for offline builds #2963

Merged
merged 2 commits into from Jan 30, 2024
Merged

Conversation

knobix
Copy link
Contributor

@knobix knobix commented Jan 23, 2024

Downloading files during build time is a non-starter for FreeBSD ports (and presumably for other *BSD ports and some Linux distros as well).

In order to still be able to build Anki successfully, a few new environment variables have been added that can be set accordingly:

  • NO_BUNDLE: If set, the creation of bundles is skipped.

  • NO_VENV: If set, the Python system environment is used instead of a venv. This is necessary if there are no usable Python wheels for a platform, e.g. PyQt6.

  • OFFLINE_BUILD: If set, the git repository synchronization (translation files, build hash, etc.) is skipped.

  • OFFLINE_YARNCACHE: Path to the offline cache that is being used by yarn(1).

To successfully build Anki offline, following conditions must be met:

  1. All required dependencies (node, Python, rust, yarn, etc.) must be present in the build environment.

  2. The offline repositories for the translation files must be copied/linked to ftl/qt-repo and ftl/core-repo.

  3. The Python pseudo venv needs to be setup:

    $ mkdir out/pyenv/bin $ ln -s /path/to/python out/pyenv/bin/python
    $ ln -s /path/to/protoc-gen-mypy out/pyenv/bin/protoc-gen-mypy

  4. Create the offline cache for yarn and point OFFLINE_YARNCACHE to it:

    OFFLINE_YARNCACHE=/path/to/the/yarn/cache $ /path/to/yarn --cache-folder $OFFLINE_YARNCACHE install --ignore-scripts

  5. Build Anki:

    $ /path/to/cargo build --package runner --release --verbose --verbose $ OFFLINE_BUILD=1 \ OFFLINE_YARNCACHE=/path/to/the/yarn/cache \ NO_BUNDLE=1 \ NO_VENV=1 \ ${WRKSRC}/out/rust/release/runner build wheels

@knobix knobix changed the title Add support for offline builds DRAFT_Add support for offline builds Jan 23, 2024
@knobix
Copy link
Contributor Author

knobix commented Jan 23, 2024

The pull request contains a a sub-set of the patches in the FreeBSD ports tree: https://github.com/freebsd/freebsd-ports/tree/main/games/anki/files.

The CI failure in this state isn't very surprising because this is currently more of a hack to get a successful build of Anki on FreeBSD. If there's enough interest, I'll try to get a successful CI build (which might take some time) or someone from the project will make the necessary adjustments/changes to make a merge possible (which might be faster).
I was able to fix the CI issues.

P.S.: Although it is my first pull request for this project, I have left out an entry in CONTRIBUTORS for the time being because as Anki's own code isn't affected by this as far I can tell. If it is still desired, I will be happy to add my name later.
As it's my first contribution and the CI checks for an entry by using the e-mail address of the commits I added my name to the contributor list as well to get a successful build.

@knobix knobix marked this pull request as draft January 23, 2024 20:06
@knobix knobix marked this pull request as ready for review January 24, 2024 19:04
@knobix knobix changed the title DRAFT_Add support for offline builds Add support for offline builds Jan 24, 2024
@dae
Copy link
Member

dae commented Jan 25, 2024

Hi Kai,

I'm ok with merging distro-related changes when they're unobtrusive and can't be easily avoided. Could you clarify some things for me though please?

  • Can't NO_VENV be avoided by creating the venv folder manually prior to invoking ninja?
  • IIRC build_bundle() only writes actions to build.ninja, and doesn't do anything on its own. Can't you just avoid calling ninja with those targets?
  • Would it make sense to drop OFFLINE_YARNCACHE and use OFFLINE_BUILD to alter the behavior of yarn as well?
  • How about adding offline: bool to SyncSubmodule instead of creating a separate command?

@knobix
Copy link
Contributor Author

knobix commented Jan 25, 2024

Hi Damien,

thank you for the feedback. I'll try to answer your questions as best I can.

* Can't NO_VENV be avoided by creating the venv folder manually prior to invoking ninja?

For the FreeBSD port in its current state, the folder structure for the "pseudo" venv is created accordingly in advance (see also https://github.com/freebsd/freebsd-ports/blob/main/games/anki/Makefile#L112). I tried again with NO_VENV=1 so that setup_venv() was executed as usual. But then pip-sync is also executed, which in turn requires access to the Internet.

I also initially considered adding some sort of offline: bool, like you mentioned it for SyncSubmodule, for the PythonEnvironment struct to avoid executing runner pyenv (see https://github.com/ankitects/anki/blob/main/build/ninja_gen/src/python.rs#L93) which executes pip.

But this seemed too invasive regarding the code of runner to me and because I'm not very savy with Rust, the variant with the NO_VENV and the stub routines was a more usable approach for me.

* IIRC build_bundle() only writes actions to build.ninja, and doesn't do anything on its own. Can't you just avoid calling ninja with those targets?

I tried it without NO_BUNDLE and can still build the FreeBSD port of Anki without any problems. Therefore, NO_BUNDLE can be left out and is probably a leftover from when I was working intensively on porting Anki (with the runner build system) for FreeBSD. I'll remove the related parts of code in this PR with the next git push.

* Would it make sense to drop OFFLINE_YARNCACHE and use OFFLINE_BUILD to alter the behavior of yarn as well?

I had originally thought that OFFLINE_BUILD was for disabling any functions (git, yarn, pip, etc.) that require access to the Internet. OFFLINE_YARNCACHE, on the other hand, is an additional variable for yarn that is only used when OFFLINE_BUILD is active.

I admit that a better approach would be like this (in pseudo code):

yarn_args = [ "install" ]

if (OFFLINE_BUILD)
    yarn_args.add("--offline", "--ignore-scripts")

    if (OFFLINE_YARNCACHE)
        yarn_args.add("--cache-folder OFFLINE_YARNCACHE)

execute("yarn", yarn_args)
* How about adding offline: bool to SyncSubmodule instead of creating a separate command?

That would of course be cleaner and the code would also be reduced in some way. I'm happy to give it a try, but please bear with me, I'm not very savy with Rust, as I already mentioned it earlier.

@dae
Copy link
Member

dae commented Jan 25, 2024

Hi Kai,

  • NO_VENV: ok, sounds like we need to keep that one
  • OFFLINE_YARNCACHE: under what circumstances would a user want an offline build but not an offline yarn cache? If they're typically used together, why not combine them into one option?
  • If you could merge SyncSubmodule and the offline variant that would be good - it should be straightforward even with little Rust experience.

@knobix
Copy link
Contributor Author

knobix commented Jan 27, 2024

Hi Damien,

thank you for the suggestions. I've updated the PR and it should contain following changes:

  • NO_BUNDLE has been removed as it's not required.

  • SyncSubmodule has now offline_build and SyncSubmoduleOffline has been removed accordingly.

  • setup_sphix() needs also to be skipped for offline builds because it invokes pip.

  • Regarding OFFLINE_YARNCACHE:

* OFFLINE_YARNCACHE: under what circumstances would a user want an offline build but not an offline yarn cache? If they're typically used together, why not combine them into one option?

I did some more research and found that the offline cache for yarn can also be defined by its own environment variable YARN_CACHE_FOLDER. This is exactly what OFFLINE_YARNCACHE was actually intended for: To be able to set the path for the offline cache specifically.

I have therefore also removed the parameter --cache-folder, which was previously used if OFFLINE_YARNCACHE was set, for the sake of completeness.

In short: There's only OFFLINE_BUILD now.

At last but not least: The commit message has been updated as well to reflect the changes.

Copy link
Member

@dae dae left a comment

Choose a reason for hiding this comment

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

Just some minor tweaks:

build/configure/src/bundle.rs Outdated Show resolved Hide resolved
build/configure/src/rust.rs Outdated Show resolved Hide resolved
build/ninja_gen/src/git.rs Outdated Show resolved Hide resolved
@knobix
Copy link
Contributor Author

knobix commented Jan 29, 2024

Just some minor tweaks:

Thank you for your suggestions, which have now been implemented accordingly.

@dae
Copy link
Member

dae commented Jan 30, 2024

Thanks Kai. This looks all ready to go, except it currently has a conflict. Could you please rebase your changes over the latest main? Then I'll be able to merge them in.

Downloading files during build time is a non-starter for FreeBSD ports
(and presumably for other *BSD ports and some Linux distros as well).

In order to still be able to build Anki successfully, two new
environment variables have been added that can be set accordingly:

* NO_VENV: If set, the Python system environment is used instead of
  a venv. This is necessary if there are no usable Python wheels for a
  platform, e.g. PyQt6.

* OFFLINE_BUILD: If set, the git repository synchronization (translation
  files, build hash, etc.) is skipped.

To successfully build Anki offline, following conditions must be met:

1. All required dependencies (node, Python, rust, yarn, etc.) must be
   present in the build environment.

2. The offline repositories for the translation files must be
   copied/linked to ftl/qt-repo and ftl/core-repo.

3. The Python pseudo venv needs to be setup:

   $ mkdir out/pyenv/bin
   $ ln -s /path/to/python out/pyenv/bin/python
   $ ln -s /path/to/protoc-gen-mypy out/pyenv/bin/protoc-gen-mypy

4. Create the offline cache for yarn and use its own environment
   variable YARN_CACHE_FOLDER to it:

   YARN_CACHE_FOLDER=/path/to/the/yarn/cache
   $ /path/to/yarn install --ignore-scripts

5. Build Anki:

   $ /path/to/cargo build --package runner --release --verbose --verbose
   $ OFFLINE_BUILD=1 \
     NO_VENV=1 \
     ${WRKSRC}/out/rust/release/runner build wheels
@knobix
Copy link
Contributor Author

knobix commented Jan 30, 2024

Thanks Kai. This looks all ready to go, except it currently has a conflict. Could you please rebase your changes over the latest main? Then I'll be able to merge them in.

No problem, Damien. Now it should be all set.

@dae dae merged commit 42cc2c9 into ankitects:main Jan 30, 2024
1 check passed
Staudey added a commit to Staudey/packages that referenced this pull request Feb 9, 2024
Staudey added a commit to Staudey/packages that referenced this pull request Feb 9, 2024
Staudey added a commit to Staudey/packages that referenced this pull request Feb 9, 2024
**Summary**

Release notes:
- [2.1.55](https://github.com/ankitects/anki/releases/tag/2.1.55)
- [2.1.56](https://github.com/ankitects/anki/releases/tag/2.1.56)
- [2.1.57](https://github.com/ankitects/anki/releases/tag/2.1.57)
- [2.1.58](https://github.com/ankitects/anki/releases/tag/2.1.58)
- [2.1.59](https://github.com/ankitects/anki/releases/tag/2.1.59)
- [2.1.60](https://github.com/ankitects/anki/releases/tag/2.1.60)
- [2.1.61](https://github.com/ankitects/anki/releases/tag/2.1.61)
- [2.1.62](https://github.com/ankitects/anki/releases/tag/2.1.62)
- [2.1.63](https://github.com/ankitects/anki/releases/tag/2.1.63)
- [2.1.64](https://github.com/ankitects/anki/releases/tag/2.1.64)
- [2.1.65](https://github.com/ankitects/anki/releases/tag/2.1.65)
- [2.1.66](https://github.com/ankitects/anki/releases/tag/2.1.66)
- [23.10](https://github.com/ankitects/anki/releases/tag/23.10)
- [23.10.1](https://github.com/ankitects/anki/releases/tag/23.10.1)
- [23.12](https://github.com/ankitects/anki/releases/tag/23.12)
- [23.12.1](https://github.com/ankitects/anki/releases/tag/23.12.1)

**Packaging notes:** Anki (once again) changed their whole build system and (once again) downloads many build dependencies instead of using local ones.
It also pins them to very specific versions. Recently [a PR was merged that enables some more local deps and offline builds](ankitects/anki#2963), but this didn't quite work right yet in my testing.

Signed-off-by: Thomas Staudinger <Staudi.Kaos@gmail.com>
ReillyBrogan added a commit to getsolus/packages that referenced this pull request Feb 9, 2024
**Summary**

Release notes:
- [2.1.55](https://github.com/ankitects/anki/releases/tag/2.1.55) |
[2.1.56](https://github.com/ankitects/anki/releases/tag/2.1.56) |
[2.1.57](https://github.com/ankitects/anki/releases/tag/2.1.57) |
[2.1.58](https://github.com/ankitects/anki/releases/tag/2.1.58) |
[2.1.59](https://github.com/ankitects/anki/releases/tag/2.1.59)
- [2.1.60](https://github.com/ankitects/anki/releases/tag/2.1.60) |
[2.1.61](https://github.com/ankitects/anki/releases/tag/2.1.61) |
[2.1.62](https://github.com/ankitects/anki/releases/tag/2.1.62) |
[2.1.63](https://github.com/ankitects/anki/releases/tag/2.1.63) |
[2.1.64](https://github.com/ankitects/anki/releases/tag/2.1.64) |
[2.1.65](https://github.com/ankitects/anki/releases/tag/2.1.65) |
[2.1.66](https://github.com/ankitects/anki/releases/tag/2.1.66)
- [23.10](https://github.com/ankitects/anki/releases/tag/23.10) |
[23.10.1](https://github.com/ankitects/anki/releases/tag/23.10.1) |
[23.12](https://github.com/ankitects/anki/releases/tag/23.12) |
[23.12.1](https://github.com/ankitects/anki/releases/tag/23.12.1)

**Packaging notes:** Anki (once again) changed their whole build system
and (once again) downloads many build dependencies instead of using
local ones. It also pins them to very specific versions. Recently [a PR
was merged that enables some more local deps and offline
builds](ankitects/anki#2963), but this didn't
quite work right yet in my testing.

**Test Plan**

- Installed and ran in a fresh Budgie VM to confirm dependencies were
correct
- Imported a new deck
- Played through a few dozen cards with audio and pictures

**Checklist**

- [x] Package was built and tested against unstable
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.

None yet

2 participants