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

Don't set -extldflags unless LDFLAGS has a value #3545

Merged
merged 1 commit into from
Feb 25, 2019

Conversation

hartzell
Copy link

Unconditionally setting -extldflags to the contents of LDFLAGS
causes problems when LDFLAGS isn't defined (-extldflags doens't
have an corresponding value).

In out case this led to git-lfs aborting.

$ ./bin/git-lfs
Aborted

This change only sets -extldflags if LDFLAGS is defined.

Unconditionally setting `-extldflags` to the contents of `LDFLAGS`
causes problems when `LDFLAGS` isn't defined (`-extldflags` doens't
have an corresponding value).

In out case this led to `git-lfs` aborting.

```
$ ./bin/git-lfs
Aborted
```

This change only sets `-extldflags` if `LDFLAGS` is defined.
@bk2204
Copy link
Member

bk2204 commented Feb 25, 2019

Hey, thanks for the patch and welcome to Git LFS!

I think this looks obviously correct. I'm not sure why we abort in this case; it's likely due to something in Go itself on whatever version you're building on. I will admit I'm curious as to why this fails for you and what version of Go you're building against, but that shouldn't prevent us from merging this as it stands.

It looks like from the repo you've linked, that you might be building against Go 1.5. Is that correct? If so, it might be worth using a newer version, since we generally only support the latest version of Go and don't test against older versions.

@hartzell
Copy link
Author

We use the Spack package manager to build a variety of things. We recently noticed that we had builds of git-lfs that didn't work. Sometimes. Dig, dig, dig and I ended up with this.

We're building on minimal-ish CentOS 7 systems (not very many packages installed).

I'm running a fairly recent version of go, I'm not sure what you see that makes you think [email protected]:

$ which go
~/spack/opt/spack/linux-centos7-x86_64/gcc-8.2.0/go-1.11.5-pei4g2iupo6y2yvogg4vde2poix2ho7d/bin/go
$ go version
go version go1.11.5 linux/amd64
[ghartzell@bifx1n03 git-lfs]$

I'm tracking this discussion over here: spack/spack#10702

@bk2204
Copy link
Member

bk2204 commented Feb 25, 2019

I'm running a fairly recent version of go, I'm not sure what you see that makes you think [email protected]:

Ah, I saw the build-dependency listed in this file and thought it might have been an old version of Go.

I'm going to merge this, and if we do a 2.7.2 release, I'll make sure we include it.

@bk2204 bk2204 added this to the v2.7.2 milestone Feb 25, 2019
@bk2204 bk2204 merged commit 24d53cf into git-lfs:master Feb 25, 2019
@hartzell
Copy link
Author

hartzell commented Feb 25, 2019

[edit: "At" -> "Ah"]

Ah, that build dependency is [email protected] or later. The spack "concretizer" (SAT solver) will put together a graph that satisfies the various requirements/constraints, generally preferring the newest version that it knows about.

scheibelp pushed a commit to spack/spack that referenced this pull request Feb 27, 2019
Fixes #10702

Same fix merged upstream in git-lfs/git-lfs#3545 -- it may appear
in future release v2.7.2 according to package maintainer.
bk2204 added a commit that referenced this pull request Apr 4, 2019
Don't set -extldflags unless LDFLAGS has a value
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

2 participants