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

Adding Microsoft CI with Appveyor #74

Closed
wants to merge 13 commits into from
Closed

Adding Microsoft CI with Appveyor #74

wants to merge 13 commits into from

Conversation

nathaniel-brough
Copy link

  • Currently builds are successful on windows.

  • Tests work but are currently failing. e.g. TestDiffBisect

  • The "make lint" command is unlikely to work as it relies on lint.sh which windows cannot run without pulling in msys or cygwin as a dependency. I would create a ".cmd" however am not at all familiar with windows.

  • I have changed all the remote paths, to work with the sergi repository.

  • You will just need to set up an account with appveyor and attach it this repo and it should find the appveyor.yaml file and start a build on the next commit.

@maksimov
Copy link
Contributor

We would probably need to add Windows detection to the Makefile, something like this:

ifdef SYSTEMROOT
   Lint = lint.bat
else
   Lint = lint.sh
endif

lint:
	$(ROOT_DIR)/scripts/$(Lint)

Then we need to port lint.sh to Windows. I'd love to do it, but it will depend on a Windows machine availability.

@nathaniel-brough
Copy link
Author

I was actually thinking it may be a bit cleaner to just convert lint.sh to gnu make format. That way it is inherently cross platform and it's all in one place. It also kinda means you can keep the same logic and only use os specific tools when needed.

@nathaniel-brough
Copy link
Author

Ok so I have implemented a simple lint script that is now within the makefile and is cross platform. I would prefer to actually do away with the regex's in the script if that is possible? I'm not too sure how much of an issue backwards compatibility is. The only current error at the moment with the build is "method DiffPrettyHtml should be DiffPrettyHTML". I managed to get away with removing the other regex just by dropping the verbose flag on the "go tool vet".

I personally am of the opinion that simple is better. But I understand if backwards compatibility is priority. Please let me know what you think and if need be I will work on another solution.

Note: All the fgt commands you see are part of this tool, which essentially just exits with an error status if anything is printed too stdout or stderror. I have added this to the install-tools target.

@zimmski
Copy link
Collaborator

zimmski commented Apr 9, 2017

Sorry for taking ages, there are too many OSS threads in my inbox. I will take a look now

Copy link
Collaborator

@zimmski zimmski left a comment

Choose a reason for hiding this comment

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

Except for some small things it looks pretty good :-) Looking forward to the Windows runner!

Makefile Outdated
@@ -29,13 +29,23 @@ install-tools:
# Install linting tools
go get -u -v github.com/golang/lint/...
go get -u -v github.com/kisielk/errcheck/...
go get -u -v github.com/GeertJohan/fgt
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's just drop the "lint" changes and do not call the link Make target in the appveyor.yml file. The linting task is already handled perfectly in the Linux runners, so the Windows runner would not find any new problems. Let's keep the Windows support to a minimum.

appveyor.yml Outdated

platform: x64

branches:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove the restriction to only build the master branch.

appveyor.yml Outdated
- C:\MinGW\bin\mingw32-make install-tools
- C:\MinGW\bin\mingw32-make install-dependencies
- C:\MinGW\bin\mingw32-make test
- C:\MinGW\bin\mingw32-make lint
Copy link
Collaborator

Choose a reason for hiding this comment

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

As described above, please drop this

appveyor.yml Outdated
- cd c:\gopath\src\github.com\sergi\go-diff\
- C:\MinGW\bin\mingw32-make install-tools
- C:\MinGW\bin\mingw32-make install-dependencies
- C:\MinGW\bin\mingw32-make test
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's through in a C:\MinGW\bin\mingw32-make install before test too, I know that go test auto builds the packages, but I had builds where install cough problems on its own.

@nathaniel-brough
Copy link
Author

nathaniel-brough commented Apr 17, 2017

@zimmski I have made the requested changes although. Build is however failing on windows, due to tests. Which is interesting in itself. See here

@sergi sergi closed this Nov 19, 2019
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.

4 participants