py/makeversionhdr.py: pass --git-dir ./.git

[ffontaine]

micropython can be embedded in a more global git project such as
buildroot. In this case, micropython retrieves the version of this
global project. Fix this issue by passing --git-dir ./.git to ensure
that git commands take the current working directory as the git
directory

Signed-off-by: Fabrice Fontaine fontaine.fabrice@gmail.com

[dpgeorge]

Thanks for the patch.

micropython retrieves the version of this global project

I'm not familiar with buildroot. Can you please explain why git doesn't look up the inner-most repo and instead goes to the global one?

[ffontaine]

git can not get the version of micropython because buildroot downloads the tarball from https://github.com/micropython/micropython/releases/download/v1.9.4/micropython-1.9.4.tar.gz. This tarball does not contain any git repository. As a result, the git commands retrieve the version from the upper (and only) git repo (which is buildroot).

Currently, we have the following patch on buildroot : https://git.buildroot.net/buildroot/tree/package/micropython/0001-fix-version.patch

However, this solution can't be merged upstream as it completely disables git. I would like to keep non-upstreamable patches as low as possible in buildroot hence this PR.

[dpgeorge]

Ok, thanks for the explanation. So the issue arises when the uPy git repo is not there, but a more global one is.

Unfortunately the patch here does not work (even though the CI passes) because all code is built in a subdirectory (eg mpy-cross/) and ./.git only exists in the top-level directory. I'm not sure what a good way to fix it is.

[ffontaine]

I think there is two options:

• give the correct directory to --git-dir (i.e. ../.git) ?
• add a Makefile option to allow the user to disable those git commands

[stinos]

A relative path seems like a bad idea, who knows what the working directory could be. So a way to override this via the build system seems more interesting?

[dpgeorge]

A relative path seems like a bad idea, who knows what the working directory could be. So a way to override this via the build system seems more interesting?

It could be possible in the Makefile to use the existing $(TOP) variable and pass it into the script.

[stinos]

Indeed, but it might still be interesting to have this as a separate Makefile variable which can be overridden. I can imagine if people build MicroPython as part of something bigger they want it to have the same version (i.e. the 'problem' which gets fixed here can be a feuture for some).

[ffontaine]

I updated the PR, now there is a PY_GIT_PATH variable that is set by default to $(TOP)/.git

[stinos]

To make this work for the msvc port you need similar changes in genhdr.targets: under the first PropertyGroup insert on line 13 <PyGitDir>$(PyBaseDir).git</PyGitDir> (I'm using dir name instead of path because it matches the argument name --git-dir and because it's less confusing imo since 'git path' to me sounds too much like 'path to git executable') and then add that one to the makeversionhdr invokation in line 115 like

<Exec Command="$(PyPython) $(PySrcDir)makeversionhdr.py $(TmpFile) $(PyGitDir)"/>

[ffontaine]

Thanks for your review, here is a new iteration.

[dpgeorge]

Thanks for updating the PR. It looks good, except that I would suggest that the variable names be changed to TOP_GIT_DIR (py.mk) and TopGitDir (msvc). This is because a "PY" prefix means that the variable relates to the py/ directory in some way.

If you agree (or have a better suggestion), I'm happy to make the change during merge.

[ffontaine]

I agree with your suggestion.

[dpgeorge]

I just noticed that it's possible to specify the git dir with the GIT_DIR env variable. Eg something like:

$ GIT_DIR=. make -C ports/unix

That solution doesn't require any changes here, and is possibly more flexible...?

[ffontaine]

The only drawback is that we'll have to set this variable in our buildroot file (in micropython.mk). I'll check if this is "acceptable" for the other buildroot contributors and I'll get back to you. Can you keep this PR open until then?

[dpgeorge]

The only drawback is that we'll have to set this variable in our buildroot file

Yes, it's not as "automagic" as the solution in this PR. But less code (here in this repo I mean) is easier to maintain :)

Can you keep this PR open until then?

For sure. I was about to merge it but noticed this other solution.

[dpgeorge]

@ffontaine did you find out if the simpler solution is acceptable?

[ffontaine]

Simpler solution has been merged in buildroot, so I'll close this PR, thanks for your support.

[dpgeorge]

Good to know!