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

fix(build): Fix version detection in sparse git checkouts (#803) #818

Merged
merged 1 commit into from
Feb 5, 2020

Conversation

simoncozens
Copy link
Member

Previous fix got us into the case statement but didn't provide any useful information once we got there. This provides something, at least.

Previous fix got us into the case statement but didn't provide any useful information once we got there. This provides *something*, at least.
@@ -152,7 +152,8 @@ then
# derive a version string.
elif test "`git log -1 --pretty=format:x . 2>/dev/null`" = x \
&& v=`git describe --tags --abbrev=7 --match="$prefix*" HEAD 2>/dev/null \
|| git describe --tags --abbrev=7 HEAD 2>/dev/null` \
|| git describe --tags --abbrev=7 HEAD 2>/dev/null \
|| git log -1 --pretty=format:'v0-HEAD-%h' 2>/dev/null` \
Copy link
Member

Choose a reason for hiding this comment

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

This won't work on some kinds of checkouts. I'll think about this one a bit...

Copy link
Contributor

@rjmunro rjmunro Feb 3, 2020

Choose a reason for hiding this comment

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

git describe has an --always flag that returns a bare SHA if there is nothing else available. This might be useful:

$ git describe
fatal: No names found, cannot describe anything.
$ git describe --always
12d7610

Copy link
Contributor

Choose a reason for hiding this comment

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

Also you may be able do a git fetch --depth 100 or some number that is likely to be more than the time since the last release to try to get the tag.

Copy link
Member

@alerque alerque Feb 3, 2020

Choose a reason for hiding this comment

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

Doing a fetch in the build system is problematic because some environments don't have access to the credentials or network access for it. Also I'm not sure --always is what we want here because this is supposed to fail if we aren't sure what we are. I actually had to take it off of here to solve a different problem.

Copy link
Member

Choose a reason for hiding this comment

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

Okay I've done a bit of poking and --always does break what this is supposed to do in some scenarios, and ding a git fetch from inside our build system is out of the question, but this change as presented doesn't do any harm. It's slightly more information about a broken build environment than we would have otherwise. I still think the real solution is to make sure git based builds are never shallow clones, but this is all we can do from inside the SILE source.

@@ -152,7 +152,8 @@ then
# derive a version string.
elif test "`git log -1 --pretty=format:x . 2>/dev/null`" = x \
&& v=`git describe --tags --abbrev=7 --match="$prefix*" HEAD 2>/dev/null \
|| git describe --tags --abbrev=7 HEAD 2>/dev/null` \
|| git describe --tags --abbrev=7 HEAD 2>/dev/null \
|| git log -1 --pretty=format:'v0-HEAD-%h' 2>/dev/null` \
Copy link
Member

Choose a reason for hiding this comment

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

Okay I've done a bit of poking and --always does break what this is supposed to do in some scenarios, and ding a git fetch from inside our build system is out of the question, but this change as presented doesn't do any harm. It's slightly more information about a broken build environment than we would have otherwise. I still think the real solution is to make sure git based builds are never shallow clones, but this is all we can do from inside the SILE source.

@alerque alerque added enhancement Software improvement or feature request tooling Build tooling, release management, and packaging processes labels Feb 5, 2020
@alerque alerque added this to the v0.10.4 milestone Feb 5, 2020
@alerque alerque merged commit dcd0023 into master Feb 5, 2020
@alerque alerque deleted the workaround-homebrew-HEAD branch February 5, 2020 08:49
@rjmunro
Copy link
Contributor

rjmunro commented Feb 5, 2020

@alerque

Doing a fetch in the build system is problematic because some environments don't have access to the credentials or network access for it.

It's a public project. No credentials are needed (as long as you have used the http url to clone it).

Some CI environments don't have network access? That sounds very odd - how do they get the code in the first place? Also you could use a || in that case...

@alerque
Copy link
Member

alerque commented Feb 5, 2020

It's a public project. No credentials are needed (as long as you have used the http url to clone it).

@rjmunro That's not the issue. Some build systems insist that builds work offline. The build bot downloads the sources, puts them inside a chroot or container, then executes the build scripts. More and more build systems are insisting that the process work offline as a way of ensuring isolation, than only the approved checksummed code is being used, etc.

We currently fall short of this in another area: the test suite tries to download fonts that are not in the source tarball (see #772) but it is possible to build offline. Doing a git fetch (even if from a public HTTPS url) as part of the make system is not an option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Software improvement or feature request tooling Build tooling, release management, and packaging processes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants