Skip to content

Conversation

@DanCech
Copy link
Contributor

@DanCech DanCech commented Dec 12, 2017

No description provided.

@Dieterbe
Copy link
Contributor

Dieterbe commented Dec 13, 2017

for context of what happened in nsq, see nsqio/nsq#338
when we imported the nsq code - aug 4, 2014 via 293ca31 - it had already incorporated these changes, we just didn't copy over all files, probably because i never initially cared about windows support.
in particular we didn't copy nsqd/rename_windows.go, which at the time was the right implementation for windows (and the reason rename.go was protected with the !windows build flag is because doing that on windows resulted in all kinds of trouble, as the nsq PR explains)

However, digging a bit more through the nsq history, I found nsqio/nsq@95dac2a
which shows the go stdlib now solves this for us - as of 1.5 -, so we can most likely just remove all atomic_rename stuff in favor of simply os.Rename and it should work fine.
for more info see PR nsqio/nsq#844 and golang/go#8914 (comment)
I had some trouble verifying this (finding the relevant ticket or changelog)
and https://golang.org/doc/go1.5 doesn't mention this. https://golang.org/pkg/os/#Rename mentions nothing about windows, whereas other functions have documented windows exceptions (but it doesn't mention atomicity at all?)

if you want to increase confidence, you could go through nsq's changelog to see if anything else interesting changed (and/or see if we can just pull in more recent diskqueue code), or dig further into go's stdlib/docs/tickets

@Dieterbe Dieterbe self-requested a review December 13, 2017 14:15
Copy link
Contributor

@Dieterbe Dieterbe left a comment

Choose a reason for hiding this comment

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

silly github, i already left a comment

Dieterbe added a commit that referenced this pull request Dec 15, 2017
this should make windows builds work.
replaces #245
@Dieterbe Dieterbe closed this Dec 15, 2017
@DanCech
Copy link
Contributor Author

DanCech commented Dec 15, 2017

Uh, what about the .gitignore and Makefile changes? want me to rebase this?

@DanCech
Copy link
Contributor Author

DanCech commented Dec 15, 2017

Rebased it

@Dieterbe
Copy link
Contributor

thanks Dan, sorry. moved a bit too quick on this.

@Dieterbe
Copy link
Contributor

merged 👍

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.

2 participants