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

Better experience for shallow repository #534

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

masahir0y
Copy link

The current assumption is that, if the .git directory exists, at least one tag must exist. Otherwise, builds fail and the reason of the failure is not clear.

From users' perspective, having the full git history is not necessary. In fact, shallow clone (git clone --depth=) is often used by umbrella projects since having the HEAD is enough for compilation.

This pull request fixes the broken build, and makes version string deterministic.

Cloning the repository with the --depth option is useful when you do
not need the full git history, but it fails to build core/version.c

  $ git clone --depth 1 git://git.ipxe.org/ipxe.git
  $ cd ipxe/src
  $ make
   [snip]
      [VERSION] bin/version.ipxe.dsk.o
  In file included from include/ipxe/features.h:6,
                   from core/version.c:33:
  <command-line>: error: unsigned conversion from ‘int’ to ‘unsigned char’ changes value from ‘9062’ to ‘102’ [-Werror=overflow]
  include/ipxe/dhcp.h:529:58: note: in definition of macro ‘DHCP_OPTION’
    529 | #define DHCP_OPTION( ... ) VA_ARG_COUNT ( __VA_ARGS__ ), __VA_ARGS__
        |                                                          ^~~~~~~~~~~
    ...

A shallow-cloned repository may not have any tag at all, then it returns
only a commit hash.

  $ git describe --tags --always --long --abbrev=1 --match "v*"
  9062

In such a case, VERSION_MINOR becomes empty, causing the build error.

If 'git describe' does not find any tag, let the version macros fall
back to the default:

  VERSION_MAJOR   = 1
  VERSION_MINOR   = 0
  VERSION_PATCH   = 0
  EXTRAVERSION    = +

Setting sha1 to GITVERSION is still useful.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
git-describe(3) says:

  --abbrev=<n>
      Instead of using the default 7 hexadecimal digits as the
      abbreviated object name, use <n> digits, or as many digits
      as needed to form a unique object name. An <n> of 0 will
      suppress long format, only showing the closest tag.

The number of "digits needed to form a unique object name" depends
on your git repository, e.g. 1) how you cloned it, 2) how many local
branches you have, etc.

In the full cloned repository, --abbrev=1 for v1.21.1 gave me 5-digits
as of writing (but it may increase in the future):

  $ git checkout v1.21.1
  $ git describe --tags --always --long --abbrev=1 --match 'v*'
  v1.21.1-0-g988d2

In the repository cloned with --depth 1000, it is 4-digits.

  $ git clone --depth 1000 git://git.ipxe.org/ipxe.git
  $ ipxe
  $ git checkout v1.21.1
  $ git describe --tags --always --long --abbrev=1 --match 'v*'
  v1.21.1-0-g988d

To make the version string deterministic, use --abbrev=32 and then
take the first 7 characters, which is reasonable for this project.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
@stappersg

This comment was marked as abuse.

@masahir0y
Copy link
Author

Any feedback please?

@NiKiZe
Copy link
Contributor

NiKiZe commented Mar 1, 2022

I agree that having a shallow clone just fail is not good, but on the other hand there is other configs in iPXE that fails builds without a clear reason as well. Guarding for everything is not possible.

Do we really want to support these builds?

What is the result (VERSION) with this patch using shallow and full clone?

@Natureshadow
Copy link

I also got hit by this today when building in GitLab CI, in a project where ipxe is a submodule.

Copy link
Contributor

@stappersg stappersg left a comment

Choose a reason for hiding this comment

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

Nice, thanks.

@stappersg

This comment was marked as abuse.

@masahir0y
Copy link
Author

I cannot believe @mcb30 in #454

It is just stupid to require the entire git history.
git clone --depth=1 is useful.
This is so obvious and easy to fix, but still not pulled.

@NiKiZe
Copy link
Contributor

NiKiZe commented Sep 3, 2022

I cannot believe @mcb30 in #454

It is just stupid to require the entire git history. git clone --depth=1 is useful. This is so obvious and easy to fix, but still not pulled.

Would still be good to have you document how this changes the version number in the different scenarios.

@stappersg

This comment was marked as abuse.

@ErikTempelaarVO
Copy link

I was trying a build in Azure Devops CI and it failed with errors like in #974, which led me here (eventually)
I was able to work around it by disabling shallow fetch for the pipeline, but a fix or a clearer error would have been very helpful.

@stappersg

This comment was marked as abuse.

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

6 participants