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.

Sign up for GitHub

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

Jump to bottom

backend: Get flannel building on windows with stubs #879

Merged
merged 1 commit into from
Nov 24, 2017
Merged

backend: Get flannel building on windows with stubs #879

merged 1 commit into from
Nov 24, 2017

Conversation

jroggeman
Copy link
Contributor

@jroggeman jroggeman commented Nov 14, 2017 •
edited
Loading

Description

Gets flannel building on windows; the second PR as described in #832.

  • Marks most of the linux-specific files as // +build !windows
  • Adds stub implementations of platform-specific interfaces for windows
  • Adds simple appveyor file to use for automated windows tests (on top of Travis)

@rakelkar
Copy link
Contributor

@tomdee

We've added an appveyor build YML to enable CI - however to work you need enable it for the repo in repo settings. Its free for open source projects and is what was just done to get the CNI repo to do CI for windows, see containernetworking/plugins#84

We also preferred adding !windows instead of naming the files _linux.. this leaves them free to compile on *nix and only windows code will be _windows (from subsequent commits) - this feels cleaner...

@tomdee
Copy link
Contributor

tomdee commented Nov 16, 2017

Looking good, but this will need a few changes before I can merge:

  • Rebase - network/ipmasq* files have been renamed in master
  • Make sure you have newlines after the build directive lines (use go vet to spot places where this is wrong).
  • Can you put the build tags under the copywright statements.

I'll take a look at enabling appveyor on this repo

@jroggeman jroggeman force-pushed the jroggeman/build_windows branch 5 times, most recently from 597d80b to 77a99f5 Compare November 21, 2017 23:26
@jroggeman
Copy link
Contributor Author

@tomdee Made changes, should be good to go

tomdee
tomdee reviewed Nov 22, 2017
View reviewed changes
backend/gce/metadata.go
@@ -1,3 +1,5 @@
// +build !windows
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove this extra build statement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed the above one to remain consistent with style

tomdee
tomdee reviewed Nov 22, 2017
View reviewed changes
backend/udp/udp.go Outdated
@@ -11,7 +11,7 @@
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// +build !amd64
// +build !amd64,!windows
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the !windows be on a new line?

tomdee
tomdee reviewed Nov 22, 2017
View reviewed changes
backend/udp/udp_windows.go Outdated
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
// +build windows
Copy link
Contributor

Choose a reason for hiding this comment

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

Do the _windows files need a windows build tag?

@tomdee
Copy link
Contributor

tomdee commented Nov 22, 2017

What are the changes to the vendor/* files?

@jroggeman
Copy link
Contributor Author

vendor files were whitespace changes from gofmt; I can revert them.

@jroggeman jroggeman force-pushed the jroggeman/build_windows branch from 77a99f5 to 145a4a4 Compare November 22, 2017 21:58
@tomdee
Copy link
Contributor

tomdee commented Nov 24, 2017

Looks good, merging.

@tomdee tomdee merged commit 1b1461a into flannel-io:master Nov 24, 2017
@rakelkar
Copy link
Contributor

@tomdee Thanks for reviewing and merging. Can enable AppVeyor CI to make sure it keeps building?

@rakelkar
Copy link
Contributor

#833

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomdee tomdee tomdee left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jroggeman @rakelkar @tomdee