Custom LFS integration has broken unexpectedly

[eloquence]

Details

The SecureDrop project uses https://github.com/freedomofpress/securedrop-docs as its documentation repo. It uses LFS because screenshots are updated with each release, which would otherwise balloon the repo size.

We are aware that LFS is not officially supported (#1846), but have so far relied on a commonly used method for installing git-lfs via conf.py (https://github.com/freedomofpress/securedrop-docs/blob/main/docs/conf.py#L21-L27). This method has stopped working unexpectedly approximately 2-3 days ago, causing all images in our documentation to break (they are served as LFS pointers).

We would appreciate any light you can shed on changes to the RTD build environment that may have caused this breakage. We have attempted to resolve it using various strategies, e.g., by installing git-lfs as a package, to no avail so far. It appears to us that even when the LFS checkout is working as expected, at some point during the build process, the actual images are not copied correctly.

Further notes on our technical investigation so far can be found at freedomofpress/securedrop-docs#234 . We can of course revert to a non-LFS repo, but if there is a straightforward fix or lead for us to pursue, we would appreciate any pointers.

[humitos]

@eloquence Hi! Can you paste the link for your RTD project?

This method has stopped working unexpectedly approximately 2-3 days ago, causing all images in our documentation to break (they are served as LFS pointers).

How did it break? Can you paste the link for a failing build?

[humitos]

Definitely, something broke for some reason. The example from test-builds is not working anymore: https://test-builds.readthedocs.io/en/git-lfs/ (note the image is not shown)

[humitos]

I updated git-lfs to its latest version and it didn't fix the issue.

[humitos]

I don't have too much experience with git-lfs, but in any case, I don't think this is a bug/issue/problem of RTD --it seems the files are not properly checked out locally by git-lfs, since my test from that page shows the content as a text file:

>>> print(open('img/git-lfs.png', 'rb').read())
b'version https://git-lfs.github.com/spec/v1\noid sha256:daf4c09c20b1ed6d72f7fee66b68005abe21bfa51c692f717fd32a313fb300ab\nsize 329787\n'

when it should show the PNG content itself.

[rmol]

@humitos If you add that test read of the file to conf.py after the LFS setup, I think it will show the image content (probably causing an exception), not the LFS pointer. I found that git lfs checkout was working, but somewhere in the build, the images are being reverted to pointers. I haven't yet been able to find where that's happening, but the Sphinx code that copies images around, at least, doesn't seem to have changed recently.

[dokempf]

I have the exact same problem and I can confirm what @rmol says. My log indicates that the LFS checkout was successful, still the files are pointers when the actual sphinx-build call happens. One thing I noticed is that since my last passing build and today, two additional stages were added to the build log (they show up as git rev-parse HEAD which would only be logging the HEAD SHA1, but may be more is happening at these stage). I can tell that the RTD-generated part of conf.py has not changed between the two build.

[ruro]

I think, that I am having the same issue here.

Also, rebuilding an older version that had working LFS images breaks them, so this is almost certainly a regression.

[ruro]

It looks like the extra git rev-parse HEAD steps were introduced by #8263.
Accessing the commit property on a git backend runs rev-parse.

This might all be a red herring, though, since I don't see any way for rev-parse to convert the lfs files back to pointers.

---

I think, that I might have figured out, what is wrong. I tried adding some os.system calls to my docs/conf.py and I was able to get the following info:

1. git status thinks that all the files in the repository are deleted. This happens even before all the git-lfs "dark magic".

2. The reason, why git status thinks that all the files were deleted is because for some reason git thinks that the repository worktree root is in the current directory (which is the directory with conf.py in it, docs in my case). So

   > realpath . # current working directory
   /home/docs/checkouts/readthedocs.org/user_builds/<projectname>/checkouts/latest/docs
   > git rev-parse --show-toplevel # where git thinks the root of the repository is
   /home/docs/checkouts/readthedocs.org/user_builds/<projectname>/checkouts/latest/docs
   > find ..
   ..
   ../.git
   <snip>
   ../docs
   <snip (there is no docs/.git dir btw)>

   It seems, that the repository was initially cloned into checkouts/latest/, then we cded into docs and now for some reason git thinks that checkouts/latest/docs is the root of the repo even though the .git dir is one level higher.

3. In this state, running git-lfs checkout does checkout the lfs data correctly, but it is placed in the wrong directory (relative to docs instead of relative to the root of the repo). I was able to confirm this by adding a

   os.system("cp path/to/lfs_image.jpg ../path/to/lfs_image.jpg")

   after all the os.system("./git-lfs ...") commands. After manually moving the checked out image to the correct location, it appeared in the docs.

So the only questions that remain are:

• Why does git think that the root of the repository is in checkouts/latest/docs instead of checkouts/latest?
• What changed in readthedocs to cause this issue (and how can it be fixed)?

[dokempf]

Thanks for posting your analysis, I did a bit more digging.

* Why does `git` think that the root of the repository is in `checkouts/latest/docs` instead of `checkouts/latest`?

I found

readthedocs.org/readthedocs/vcs_support/backends/git.py

Line 366 in db0c0e2

env['GIT_DIR'] = os.path.join(self.working_dir, '.git')

which looks suspicious to me - although it is 11 years old.

* What changed in `readthedocs` to cause this issue (and how can it be fixed)?

The description of #8263 states "In some places we were executing git commands outside the container" so, maybe above line was not actually used, but is now?

Unfortunately, I am missing the code base knowledge to continue from here.

[dokempf]

So, I fixed this issue for me by playing around with the working directory of the git-lfs command. I have packaged my currently working solution into a Sphinx extension: https://github.com/ssciwr/sphinx_lfs_content Maybe having the code in a Sphinx extension will make it easier to report and address issues with the non-official LFS workflow on RTD.

[ruro]

Unfortunately, it doesn't seem, that GIT_DIR is the culprit here. By inspecting the output of env

GIT_DIR=/home/docs/checkouts/readthedocs.org/user_builds/<projectname>/checkouts/latest/.git

and git rev-parse --git-dir we can see, that the .git directory is located in

/home/docs/checkouts/readthedocs.org/user_builds/<projectname>/checkouts/latest/.git

as expected. It's only the current git worktree root, that is for some reason moved to

/home/docs/checkouts/readthedocs.org/user_builds/<projectname>/checkouts/latest/docs

---

The weirdest part is that according to git worktree --list, the only existing worktree is in

/home/docs/checkouts/readthedocs.org/user_builds/<projectname>/checkouts/latest  <sha> (detached HEAD)

So at first glance it seems like the output of git rev-parse --show-toplevel contradicts the output of git worktree --list.

---

Okay. This is insane. I think, that I might have found the problem. And it might be a bug in git itself. Consider the following example

# Setup example repo
mkdir /example
cd /example
git init .
mkdir docs
cd docs

Then the following behaviour is clearly broken

git rev-parse --show-toplevel # prints /example as expected
export GIT_DIR=/example/.git
git rev-parse --show-toplevel # prints /example/docs for some reason

In fact, after exporting GIT_DIR, rev-parse --show-toplevel seems to always just return the current directory.

The only thing, that I don't understand is how/why did it work differently in the past.

[ruro]

Ah, okay. I think, it makes sense now. The #8263 must be the cause after all. Although it is exporting the correct GIT_DIR environment variable, it results in git getting confused about the location of the current worktree.

@humitos can we get a temporary quick fix for this regression, that doesn't push the GIT_DIR to the user environment?

Edit: actually, the correct solution would probably be to just export GIT_WORK_TREE alongside GIT_DIR in readthedocs/vcs_support/backends/git.py::Backend.env

Here's a relevant extract from man git-config (core.worktree)

If --git-dir or GIT_DIR is specified but none of --work-tree, GIT_WORK_TREE
and core.worktree is specified, the current working directory is regarded as the
top level of your working tree.

Edit edit: I made a PR with the proposed fix.