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

Make AppVeyor CI for Windows work again #1506

Merged
merged 2 commits into from Sep 6, 2016
Merged

Make AppVeyor CI for Windows work again #1506

merged 2 commits into from Sep 6, 2016

Conversation

sschuberth
Copy link
Member

This build the binaries and installer for now. Not running tests yet.

All the src contains is a symbolic link to the root, causing gofmt to
indefinitely recurse until it errors out. On Windows, this causes an error
message like

    GetFileAttributesEx
    src\github.com\github\git-lfs\src\github.com\github\git-lfs\src\github.com\github\git-lfs\src\github.com\github\git-lfs\src\github.com\github\git-lfs\.git\objects\pack\pack-2fc21819ce9855ebf1a1f3ac77cf464c8a4e6e42.idx:
    The system cannot find the path specified.
# that we need.
if [ -z "$GOPATH" ] && uname -s | grep -vq "_NT-"; then
# Tell MSYS to really create symbolic links.
export MSYS=winsymlinks:nativestrict
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you able to do this from within the appveyor.yml?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess so, but then calling script/bootstrap on a local Windows developer machine (from Git Bash) would not properly work unless that variable is already set.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you feel better about it, I can limit setting that variable to Windows...

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, good idea. If we're in a Windows environment while running script/bootstrap, then we can set $MSYS.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. I'll simply re-use parts of the existing check for that.

@ttaylorr
Copy link
Contributor

ttaylorr commented Sep 6, 2016

This is looking good. I left a few minor comments, mostly me asking questions about how AppVeyor works 😅 .

One thing to note is that this won't generate signed releases of Git LFS that we can upload as our official Windows installer. I don't think this is a problem for now, since getting some sort of test harness in place for verifying that the Windows installer builds correctly is definitely a step forward.

I'd love to look into how we could get AppVeyor to sign our releases for us in the future, since that's definitely something that we'd like to do.

@sschuberth
Copy link
Member Author

One thing to note is that this won't generate signed releases of Git LFS that we can upload as our official Windows installer.

The Inno Setup compiler iscc supports the /S option to specify a signing tool, so I guess that should be easy to add.

@sschuberth
Copy link
Member Author

I've pushed another version that only exports the MSYS (and now also CYGWIN) variable on Windows.

@ttaylorr
Copy link
Contributor

ttaylorr commented Sep 6, 2016

Thanks for being so quick to respond to my comments! I'm going to wait for the tests to pass and then merge this to master and test it out.

@sschuberth
Copy link
Member Author

Please do not merge yet, there's a small bug I'll fix from home in an hour or so!

AppVeyor now has Go and Git for Windows 2.x installed by default. Also, we
can tell MSYS to actually create symbolic links. Finally, let the CI
create the installer, too.
@sschuberth
Copy link
Member Author

Fixed, see the AppVeyor artifacts at https://ci.appveyor.com/project/sschuberth/git-lfs/build/1.0.19/artifacts

@ttaylorr ttaylorr merged commit f54b90f into git-lfs:master Sep 6, 2016
@sschuberth sschuberth deleted the appveyor branch September 6, 2016 17:54
@ttaylorr
Copy link
Contributor

ttaylorr commented Sep 6, 2016

@sschuberth
Copy link
Member Author

Interesting, why's this at git-lfs/git-lfs and not at https://ci.appveyor.com/project/github/git-lfs/build/artifacts?

@ttaylorr
Copy link
Contributor

ttaylorr commented Sep 6, 2016

We're planning to move to https://github.com/git-lfs soon anyway.

@sschuberth
Copy link
Member Author

Also, looking at #1505 it seems two AppVeyor builds get kicked off, continuous-integration/appveyor/branch and continuous-integration/appveyor/pr. Is this intended?

@ttaylorr
Copy link
Contributor

ttaylorr commented Sep 6, 2016

Good catch. I enabled the "Do not build feature branches with open Pull Requests" option.

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