git-lfs: add support for board documentation files #9818

jia200x · 2018-08-22T13:02:21Z

Contribution description

This PR adds support for using git-lfs for board documentation images (and related files). It's IMO a good trade-off between having files closer to the code and not bloating the repository.

This PR tracks all files undes board/<board_name>/doc folder as git-lfs files.

git-lfs in a nutshell

Assuming the git-lfs extension is installed (apt-get install git-lfs), simply install git-lfs in the RIOT repo:

git lfs install --local

Then, work with the github repo as you normally do. When pushing to origin or another remote, git-lfs will push a symlink-like file in the repo and upload the real file to another repo. This way the original repo doesn't bloat and only contains symlinks pointing to the actual file.

Github handles LFS files natively, as seen here (there is a Stored with Git LFS message).

So if git-lfs is installed, is exactly the same as using standard git. Both from the local computer and from the github.com file viewer.

FAQ

Where are LFS files stored?: Github provides a LFS server in the same repo hidden under RIOT.git/info/lfs
If I push files to my origin, will the symlink file point to my repo?: This seems to be handled by Github, the remote location is not included in the symlink file (only a hash and some references to git-lfs.github.com)
If someone doesn't have git-lfs, can still use the RIOT repo? It's possible to use the repo without Git LFS. However, make doc will fail wih a Please install git-lfs message. The new make doc target not only checks if git-lfs is installed but also executes git lfs install --local as a dependency.
How does it work with Pull Requests?: If someone uploads a picture without using git-lfs, Github will treat it as Git LFS file instead of Binary (see different files here). It's up to maintainers to make sure only Git LFS file get to boards/<board_name>/doc (see Possible improvements)

Possible improvements

Git hook for detecting if LFS is installed when changing <board_name>/doc files.

Testing procedure

Try running make doc with and without git-lfs.

Note this doesn't add <board_name>/doc to the RIOT doxygen file, it will be done in a further PR.

Issues/PRs references

None so far.

jia200x · 2018-08-22T13:07:09Z

Murdock should fail if it tries to run make doc. It's intended, and should install the git-lfs dependency before merging this PR (if accepted by the community)

jcarrano · 2018-08-22T13:26:51Z

should install the git-lfs dependency before merging this PR

I set the 'Waiting' label. I don't know how applicable it is, but at least it lets people know it depends on something else.

jia200x · 2018-08-22T13:27:44Z

I set the 'Waiting' label. I don't know how applicable it is, but at least it lets people know it depends on something else.

That's right, thanks!

jcarrano · 2018-08-22T13:28:43Z

This PR tracks all files undes board/<board_name>/doc folder as git-lfs files.

Non-binary files as well? Can we filter on extension?

jia200x · 2018-08-22T13:28:46Z

Murdock should fail if it tries to run make doc. It's intended, and should install the git-lfs dependency before merging this PR (if accepted by the community)

Oh, it passed... does Murdock run make doc?

jia200x · 2018-08-22T13:29:36Z

Non-binary files as well? Can we filter on extension?

Yes, wildcards and basic regex patterns are accepted

jcarrano · 2018-08-22T13:38:08Z

In my machine:

$ make doc
"make" -BC doc/doxygen
make[1]: Entering directory '/home/jcarrano/source/vanillaRIOT/doc/doxygen'
Please install git lfs.
make[1]: *** [Makefile:52: git-lfs-check] Error 1
make[1]: Leaving directory '/home/jcarrano/source/vanillaRIOT/doc/doxygen'
make: *** [Makefile:10: doc] Error 2

But the check script runs "fine":

Running './dist/tools/doccheck/check.sh' ✓

The reason is that doccheck does not test the exit code of make or any error other than the filters it has configured.

jia200x · 2018-08-22T13:43:14Z

The reason is that doccheck does not test the exit code of make or any error other than the filters it has configured.

I see. I will patch that

jcarrano · 2018-08-22T13:53:03Z

@jia200x I already patched it!

doc/doxygen/Makefile

PeterKietzmann · 2019-03-05T10:25:56Z

This PR has the "waiting for other PR" label set. What is it waiting for?

jcarrano · 2019-03-05T13:54:12Z

@PeterKietzmann it was waiting for #9819. If LFS was not available, the build would fail silently.

jcarrano

It needs a couple of small makefile fixes, and then it's in.

jcarrano · 2019-03-05T13:55:02Z

doc/doxygen/Makefile

+RED=\033[0;31m
+NC=\033[0m
+
+git-lfs: git-lfs-check git-lfs-install


Make these things phony.

Also, install should depend on check. With the current definitions there is no guarantee that install won't try to run before check.

danpetry · 2019-03-19T14:41:19Z

ping @jia200x

stale · 2019-09-20T14:51:22Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

chrysn

I still like this approach and would love to see it resumed.

chrysn · 2022-09-22T08:17:15Z

.gitattributes

@@ -5,3 +5,4 @@
 # when the heading is exactly 7 characters long.
 *.md conflict-marker-size=100
 *.txt conflict-marker-size=100
+boards/*/doc/* filter=lfs diff=lfs merge=lfs -text


Maybe that should say *.jpg *.png immaterial of the precise location?

jia200x added 2 commits August 22, 2018 13:26

make: add git-lfs targets

f6ffa16

.gitattributes: track doc files for every board

715d1ea

jia200x added Area: doc Area: Documentation Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 22, 2018

jia200x requested review from jcarrano, aabadie and kaspar030 August 22, 2018 13:09

jia200x added the Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR label Aug 22, 2018

jcarrano added the State: waiting for other PR State: The PR requires another PR to be merged first label Aug 22, 2018

jcarrano mentioned this pull request Aug 22, 2018

tools/doccheck: Detect when make doc fails to run. #9819

Merged

jcarrano reviewed Sep 4, 2018

View reviewed changes

doc/doxygen/Makefile Show resolved Hide resolved

miri64 added State: waiting for other PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 5, 2018

miri64 removed the State: waiting for other PR label Oct 12, 2018

jcarrano removed the State: waiting for other PR State: The PR requires another PR to be merged first label Mar 5, 2019

jcarrano suggested changes Mar 5, 2019

View reviewed changes

danpetry mentioned this pull request Mar 19, 2019

doc/mega-xplained: add JTAG/Atmel-ICE instructions #11209

Merged

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Sep 20, 2019

stale bot closed this Oct 21, 2019

chrysn reviewed Sep 22, 2022

View reviewed changes

git-lfs: add support for board documentation files #9818

git-lfs: add support for board documentation files #9818

Uh oh!

Conversation

jia200x commented Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

git-lfs in a nutshell

FAQ

Possible improvements

Testing procedure

Issues/PRs references

Uh oh!

jia200x commented Aug 22, 2018

Uh oh!

jcarrano commented Aug 22, 2018

Uh oh!

jia200x commented Aug 22, 2018

Uh oh!

jcarrano commented Aug 22, 2018

Uh oh!

jia200x commented Aug 22, 2018

Uh oh!

jia200x commented Aug 22, 2018

Uh oh!

jcarrano commented Aug 22, 2018

Uh oh!

jia200x commented Aug 22, 2018

Uh oh!

jcarrano commented Aug 22, 2018

Uh oh!

Uh oh!

PeterKietzmann commented Mar 5, 2019

Uh oh!

jcarrano commented Mar 5, 2019

Uh oh!

jcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

jcarrano Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

jcarrano Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

danpetry commented Mar 19, 2019

Uh oh!

stale bot commented Sep 20, 2019

Uh oh!

chrysn left a comment

Choose a reason for hiding this comment

Uh oh!

chrysn Sep 22, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jia200x commented Aug 22, 2018 •

edited

Loading