Skip to content

Conversation

@erikh
Copy link

@erikh erikh commented Jan 23, 2017

Over at github.com/containers/ I'm trying to incorporate various parts of docker which leverage this library. I am building on linux.

The references to go-winio do not build on linux because they have a build tag which demands windows. This library leverages go-winio, but does not have the appropriate build tags itself, which causes it to build with undefined symbols. This is unfortunately halting our test suite as a result.

Please let me know if this is unacceptable, how I can make it acceptable.

Thanks!

cc @runcom @mtrmac

@msftclas
Copy link

Hi @erikh, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

@erikh
Copy link
Author

erikh commented Jan 23, 2017

signed

Copy link
Contributor

@lowenna lowenna left a comment

Choose a reason for hiding this comment

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

Do you not need an 'unsupported.go' file that is !windows?

@erikh
Copy link
Author

erikh commented Jan 23, 2017 via email

@msftgits
Copy link
Contributor

Hi, I am closing and re-opening this PR to bump the CLA bot. Sorry for the inconvenience!

@msftgits msftgits closed this Jan 23, 2017
@msftgits msftgits reopened this Jan 23, 2017
@msftclas
Copy link

Hi @erikh, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!
We will now validate the agreement and then real humans will evaluate your PR.

TTYL, MSBOT;

@@ -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.

I don't think this file should be updated as it's autogenerated

Copy link
Author

Choose a reason for hiding this comment

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

Does this contain windows only code? If so, we should keep it from compiling.

go generate should not affect the build tag.

Copy link
Author

Choose a reason for hiding this comment

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

Oh I see. I'll fix it.

Signed-off-by: Erik Hollensbe <github@hollensbe.org>
@erikh
Copy link
Author

erikh commented Jan 24, 2017

Added it to the generator to get the best of both worlds. I have manually edited the generated source still, but it should reflect the generated version. Can you test for me? I don't have a windows development environment atm (but fwiw, I do all my hacking on linux on windows 10 :)

@lowenna
Copy link
Contributor

lowenna commented Jan 24, 2017

@erikh - Can I ask why you're even pulling a Windows-only library into Linux code though? Questioning whether making this change is really the right thing to do, and it just simply shouldn't be used on non-Windows platforms.

@erikh
Copy link
Author

erikh commented Jan 24, 2017

I believe the requirement comes from docker, but I'll double check.

@erikh
Copy link
Author

erikh commented Jan 30, 2017

Is there a practical disadvantage to doing this?

@m-kostrzewa
Copy link

@erikh FYI it builds on Windows, haven't verified if it works.

@lowenna
Copy link
Contributor

lowenna commented Mar 11, 2017

I'm closing this for now. I can't think why a Windows-specific library would need to be included in Linux, and haven't seen anything convincing to warrant updating this.

@lowenna lowenna closed this Mar 11, 2017
dcantah pushed a commit to dcantah/hcsshim that referenced this pull request Mar 17, 2021
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.

5 participants