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

Fix building on windows #352

Merged
merged 1 commit into from
Feb 23, 2021
Merged

Fix building on windows #352

merged 1 commit into from
Feb 23, 2021

Conversation

ncruces
Copy link
Contributor

@ncruces ncruces commented Jan 22, 2021

activation doesn't build on Windows because syscall.CloseOnExec takes a syscall.Handle rather than an int there.

files := make([]*os.File, 0, nfds)
for fd := listenFdsStart; fd < listenFdsStart+nfds; fd++ {
syscall.CloseOnExec(fd)
name := "LISTEN_FD_" + strconv.Itoa(fd)
offset := fd - listenFdsStart

This fixes this with a build constraint returning nil on Windows, which makes it a NOP and I guess follows the philosophy of most of the project.

@lucab
Copy link
Contributor

lucab commented Jan 22, 2021

Thanks for the PR. However I'm quite confused. The activation module bridges to the service activation part of systemd, which as far as I know is not available on anything other than Linux. How and where are you trying to use this on Windows?

@ncruces
Copy link
Contributor Author

ncruces commented Jan 22, 2021

All of this only works on Linux, but much of it builds on Windows.

Also, even on Linux, if a service is not started by systemd, there's usually a well defined behaviour.

For the activation module, no files/listeners are returned.

For another example, the daemon module builds fine on Windows, and the behaviour of SdNotify is well defined: it shall return false, nil (meaning "notification not supported", because the env var is unset).

This leads to a useful pattern that gets used on Linux, but is useful on Windows/macOS/etc also: call into go-systemd, if it somehow reports it's not running under systemd, do something else. This is used in a bunch of examples, and works fine in general.

I'm not saying henceforth you should support building on Windows, especially when the cost is high, but here the cost seems low?

It's up to you really. Feel free to drop this if you don't think you want to support this.

@lucab
Copy link
Contributor

lucab commented Jan 22, 2021

@ncruces happy to land something like this if it is useful to you :)
Although non-Linux targets are not covered by CI at this point, so it's hard to guarantee it won't accidentally regress.
I'll have a better look at this next week.
Medium term, perhaps we can use github runners to build this repo.

@ncruces
Copy link
Contributor Author

ncruces commented Jan 22, 2021

One possible goal might be just to ensure that it builds for Windows/macOS, which given Go's cross compilation support might not be hard to add to CI even if that runs on Linux.

Any Linux tests that ensure that in the absence of systemd env variables reasonable defaults are returned, are also validating behaviour on platforms that lack systemd.

But for CI to make sense, I'd have to look at why some other modules don't build.

And really, although this is useful to me, I also understand the view that, and don't want to burden you with, maintaining something that might break in the future, and is basically just a nop.

@ncruces
Copy link
Contributor Author

ncruces commented Jan 22, 2021

I went back and looked.

This works as is, macOS:

% CGO_ENABLED=0 GOOS=darwin go build ./...
go build github.com/coreos/go-systemd/v22/internal/dlopen: build constraints exclude all Go files in [REDACTED]/go-systemd/internal/dlopen

This fails, Windows:

% CGO_ENABLED=0 GOOS=windows go build ./...
go build github.com/coreos/go-systemd/v22/internal/dlopen: build constraints exclude all Go files in [REDACTED]/go-systemd/internal/dlopen
# github.com/coreos/go-systemd/v22/journal
journal/journal.go:130:12: undefined: syscall.UnixRight

So there'd be syscall.UnixRight to fix on the journal module.
There are 3 funcs to stub there, but it should be straight forward if there's interest.

@ncruces ncruces changed the title activation: build on windows build on windows Jan 23, 2021
@ncruces
Copy link
Contributor Author

ncruces commented Jan 23, 2021

Oh my! Sorry for messing this up!
I was trying to add some commits and fix commit messages (to match your contribution guidelines).

journal should also build on Windows (and macOS) as tested by running:

CGO_ENABLED=0 GOOS=darwin go build ./...
CGO_ENABLED=0 GOOS=windows go build ./...

markelog added a commit to markelog/juju that referenced this pull request Jan 27, 2021
Fixes https://bugs.launchpad.net/juju-core/+bug/1403084

There was three types of issues for running tests on windows.

`coreos/go-systemd'
Not all of the parts of systemd is buildable on windows, there is currently
a PR open for it - coreos/go-systemd#352. But even
with these changes our tests coudn't build the test suits. Since we are using
`github.com/coreos/go-systemd/v22/util` which tries to include binary files.
Solution is to exclude problematic modules on window build via comment tags

Path Sensitivity
In windows paths are case-sensitive but on unix they are not
markelog added a commit to markelog/juju that referenced this pull request Jan 27, 2021
Fixes https://bugs.launchpad.net/juju-core/+bug/1403084

There was three types of issues for running tests on windows.

`coreos/go-systemd'
Not all of the parts of systemd is buildable on windows, there is currently
a PR open for it - coreos/go-systemd#352. But even
with these changes our tests coudn't build the test suits. Since we are using
`github.com/coreos/go-systemd/v22/util` which tries to include binary files.
Solution is to exclude problematic modules on window build via comment tags

Path Sensitivity
There was some case-sensitivity issues related to macos and

CI
N
markelog added a commit to markelog/juju that referenced this pull request Jan 27, 2021
Fixes https://bugs.launchpad.net/juju-core/+bug/1403084

There was two types of issues for running tests on windows.

`coreos/go-systemd'
Not all of the parts of systemd is buildable on windows, there is currently
a PR open for it - coreos/go-systemd#352. But even
with these changes our tests coudn't build the test suits. Since we are using
`github.com/coreos/go-systemd/v22/util` which tries to include binary files.
Solution is to exclude problematic modules on window build via comment tags

CI
GitHub actions is instructed to install chocolatey, but according to CI history
tests themselves were never run on windows. Moreover, I am not sure how
`testing/mgo module can get mongo binary on windows -
https://github.com/juju/testing/blob/2be42bba85f32755618cf63d68e7b60dc5e6e911/mgo.go#L487-L491^https://github.com/juju/testing/blob/2be42bba85f32755618cf63d68e7b60dc5e6e911/mgo.go#L487-L491

I have removed chocolatey mongo installation instructions and provided a way
for excluding the mongo during the test run. Until we can clarify
`testing/mgo` issue and if we want to fix it
markelog added a commit to markelog/juju that referenced this pull request Jan 27, 2021
Fixes https://bugs.launchpad.net/juju-core/+bug/1403084

There was two types of issues for running tests on windows.

`coreos/go-systemd'
Not all of the parts of systemd is buildable on windows, there is currently
a PR open for it - coreos/go-systemd#352. But even
with these changes our tests coudn't build the test suits. Since we are using
`github.com/coreos/go-systemd/v22/util` which tries to include binary files.
Solution is to exclude problematic modules on window build via comment tags

CI
GitHub actions is instructed to install chocolatey, but according to CI history
tests themselves were never run on windows. Moreover, I am not sure how
`testing/mgo module can get mongo binary on windows -
https://github.com/juju/testing/blob/2be42bba85f32755618cf63d68e7b60dc5e6e911/mgo.go#L487-L491^https://github.com/juju/testing/blob/2be42bba85f32755618cf63d68e7b60dc5e6e911/mgo.go#L487-L491

I have removed chocolatey mongo installation instructions and provided a way
for excluding the mongo during the test run. Until we can clarify
`testing/mgo` issue and if we want to fix it
markelog added a commit to markelog/juju that referenced this pull request Jan 27, 2021
Fixes https://bugs.launchpad.net/juju-core/+bug/1403084

There was two types of issues for running tests on windows.

`coreos/go-systemd'
Not all of the parts of systemd is buildable on windows, there is currently
a PR open for it - coreos/go-systemd#352. But even
with these changes our tests coudn't build the test suits. Since we are using
`github.com/coreos/go-systemd/v22/util` which tries to include binary files.
Solution is to exclude problematic modules on window build via comment tags

CI
GitHub actions is instructed to install chocolatey, but according to CI history
tests themselves were never run on windows. Moreover, I am not sure how
`testing/mgo module can get mongo binary on windows -
https://github.com/juju/testing/blob/2be42bba85f32755618cf63d68e7b60dc5e6e911/mgo.go#L487-L491^https://github.com/juju/testing/blob/2be42bba85f32755618cf63d68e7b60dc5e6e911/mgo.go#L487-L491

I have removed chocolatey mongo installation instructions and provided a way
for excluding the mongo during the test run. Until we can clarify
`testing/mgo` issue and if we want to fix it
markelog added a commit to markelog/juju that referenced this pull request Jan 27, 2021
Fixes https://bugs.launchpad.net/juju-core/+bug/1403084

There were two types of issues for running tests on windows.

`coreos/go-systemd'
Not all of the parts of systemd is buildable on windows, there is currently
a PR open for it - coreos/go-systemd#352. But even
with these changes our tests couldn't build the test suits. Since we are using
`github.com/coreos/go-systemd/v22/util` which tries to include binary files.
Solution is to exclude problematic modules on window build via comment tags

CI
GitHub actions is instructed to install chocolatey, but according to CI history
tests themselves were never run on windows. Moreover, I am not sure how
`testing/mgo module can get mongo binary on windows -
https://github.com/juju/testing/blob/2be42bba85f32755618cf63d68e7b60dc5e6e911/mgo.go#L487-L491^https://github.com/juju/testing/blob/2be42bba85f32755618cf63d68e7b60dc5e6e911/mgo.go#L487-L491

I have removed chocolatey mongo installation instructions and provided a way
for excluding the mongo during the test run. Until we can clarify
`testing/mgo` issue and if we want to fix it
markelog added a commit to markelog/juju that referenced this pull request Jan 27, 2021
Fixes https://bugs.launchpad.net/juju-core/+bug/1403084

There were two types of issues for running tests on windows.

`coreos/go-systemd'
Not all of the parts of systemd is buildable on windows, there is currently
a PR open for it - coreos/go-systemd#352. But even
with these changes our tests couldn't build the test suits. Since we are using
`github.com/coreos/go-systemd/v22/util` which tries to include binary files.
Solution is to exclude problematic modules on window build via comment tags

CI
GitHub actions is instructed to install chocolatey, but according to CI history
tests themselves were never run on windows. Moreover, I am not sure how
`testing/mgo module can get mongo binary on windows -
https://github.com/juju/testing/blob/2be42bba85f32755618cf63d68e7b60dc5e6e911/mgo.go#L487-L491^https://github.com/juju/testing/blob/2be42bba85f32755618cf63d68e7b60dc5e6e911/mgo.go#L487-L491

I have removed chocolatey mongo installation instructions and provided a way
for excluding the mongo during the test run. Until we can clarify
`testing/mgo` issue and if we want to fix it
markelog added a commit to markelog/juju that referenced this pull request Jan 27, 2021
Fixes https://bugs.launchpad.net/juju-core/+bug/1403084

There were two types of issues for running tests on windows.

`coreos/go-systemd'
Not all of the parts of systemd is buildable on windows, there is currently
a PR open for it - coreos/go-systemd#352. But even
with these changes our tests couldn't build the test suits. Since we are using
`github.com/coreos/go-systemd/v22/util` which tries to include binary files.
Solution is to exclude problematic modules on window build via comment tags

CI
GitHub actions is instructed to install chocolatey, but according to CI history
tests themselves were never run on windows. Moreover, I am not sure how
`testing/mgo module can get mongo binary on windows -
https://github.com/juju/testing/blob/2be42bba85f32755618cf63d68e7b60dc5e6e911/mgo.go#L487-L491^https://github.com/juju/testing/blob/2be42bba85f32755618cf63d68e7b60dc5e6e911/mgo.go#L487-L491

I have removed chocolatey mongo installation instructions and provided a way
for excluding the mongo during the test run. Until we can clarify
`testing/mgo` issue and if we want to fix it
markelog added a commit to markelog/juju that referenced this pull request Jan 27, 2021
Fixes https://bugs.launchpad.net/juju-core/+bug/1403084

There were two types of issues for running tests on windows.

`coreos/go-systemd'
Not all of the parts of systemd is buildable on windows, there is currently
a PR open for it - coreos/go-systemd#352. But even
with these changes our tests couldn't build the test suits. Since we are using
`github.com/coreos/go-systemd/v22/util` which tries to include binary files.
Solution is to exclude problematic modules on window build via comment tags

CI
GitHub actions is instructed to install chocolatey, but according to CI history
tests themselves were never run on windows. Moreover, I am not sure how
`testing/mgo module can get mongo binary on windows -
https://github.com/juju/testing/blob/2be42bba85f32755618cf63d68e7b60dc5e6e911/mgo.go#L487-L491

I have removed chocolatey mongo installation instructions and provided a way
for excluding the mongo during the test run. Until we can clarify
`testing/mgo` issue and if we want to fix it
markelog added a commit to markelog/juju that referenced this pull request Jan 27, 2021
Fixes https://discourse.charmhub.io/t/building-juju-2-5-0-on-windows/549

There were two types of issues for running tests on windows.

`coreos/go-systemd'
Not all of the parts of systemd is buildable on windows, there is currently
a PR open for it - coreos/go-systemd#352. But even
with these changes our tests couldn't build the test suits. Since we are using
`github.com/coreos/go-systemd/v22/util` which tries to include binary files.
Solution is to exclude problematic modules on window build via comment tags

CI
GitHub actions is instructed to install chocolatey, but according to CI history
tests themselves were never run on windows. Moreover, I am not sure how
`testing/mgo module can get mongo binary on windows -
https://github.com/juju/testing/blob/2be42bba85f32755618cf63d68e7b60dc5e6e911/mgo.go#L487-L491

I have removed chocolatey mongo installation instructions and provided a way
for excluding the mongo during the test run. Until we can clarify
`testing/mgo` issue and if we want to fix it
markelog added a commit to markelog/juju that referenced this pull request Jan 27, 2021
Fixes https://discourse.charmhub.io/t/building-juju-2-5-0-on-windows/549

There were two types of issues for running tests on windows.

`coreos/go-systemd'
Not all of the parts of systemd is buildable on windows, there is currently
a PR open for it - coreos/go-systemd#352. But even
with these changes our tests couldn't build the test suits. Since we are using
`github.com/coreos/go-systemd/v22/util` which tries to include some os specific
binary files.

Solution is to exclude problematic modules on window build via comment tags

CI
GitHub actions is instructed to install chocolatey, but according to CI history
tests themselves were never run on windows. Moreover, I am not sure how
`testing/mgo module can get mongo binary on windows -
https://github.com/juju/testing/blob/2be42bba85f32755618cf63d68e7b60dc5e6e911/mgo.go#L487-L491

I have removed chocolatey mongo installation instructions and provided a way
for excluding the mongo during the test run. Until we can clarify
`testing/mgo` issue and if we want to fix it
markelog added a commit to markelog/juju that referenced this pull request Feb 4, 2021
Fixes https://discourse.charmhub.io/t/building-juju-2-5-0-on-windows/549

`coreos/go-systemd'
Not all of the parts of systemd is buildable on windows, there is currently
a PR open for it - coreos/go-systemd#352. But even
with these changes our tests couldn't build the test suits. Since we are using
`github.com/coreos/go-systemd/v22/util` which tries to include some os specific
binary files.

Solution is to exclude problematic modules on window build via comment tags
markelog added a commit to markelog/juju that referenced this pull request Feb 4, 2021
Fixes https://discourse.charmhub.io/t/building-juju-2-5-0-on-windows/549

`coreos/go-systemd'
Not all of the parts of systemd is buildable on windows, there is currently
a PR open for it - coreos/go-systemd#352. But even
with these changes our tests couldn't build the test suits. Since we are using
`github.com/coreos/go-systemd/v22/util` which tries to include some os specific
binary files.

Solution is to exclude problematic modules on window build via comment tags
markelog added a commit to markelog/juju that referenced this pull request Feb 4, 2021
Fixes https://discourse.charmhub.io/t/building-juju-2-5-0-on-windows/549

`coreos/go-systemd'
Not all of the parts of systemd is buildable on windows, there is currently
a PR open for it - coreos/go-systemd#352. But even
with these changes our tests couldn't build the test suits. Since we are using
`github.com/coreos/go-systemd/v22/util` which tries to include some os specific
binary files.

Solution is to exclude problematic modules on window build via comment tags
markelog added a commit to markelog/juju that referenced this pull request Feb 4, 2021
Fixes https://discourse.charmhub.io/t/building-juju-2-5-0-on-windows/549

`coreos/go-systemd'
Not all of the parts of systemd is buildable on windows, there is currently
a PR open for it - coreos/go-systemd#352. But even
with these changes our tests couldn't build the test suits. Since we are using
`github.com/coreos/go-systemd/v22/util` which tries to include some os specific
binary files.

Solution is to exclude problematic modules on window build via comment tags
markelog added a commit to markelog/juju that referenced this pull request Feb 4, 2021
Fixes https://discourse.charmhub.io/t/building-juju-2-5-0-on-windows/549

`coreos/go-systemd'
Not all of the parts of systemd is buildable on windows, there is currently
a PR open for it - coreos/go-systemd#352. But even
with these changes our tests couldn't build the test suits. Since we are using
`github.com/coreos/go-systemd/v22/util` which tries to include some os specific
binary files.

Solution is to exclude problematic modules on window build via comment tags
markelog added a commit to markelog/juju that referenced this pull request Feb 4, 2021
Fixes https://discourse.charmhub.io/t/building-juju-2-5-0-on-windows/549

`coreos/go-systemd'
Not all of the parts of systemd is buildable on windows, there is currently
a PR open for it - coreos/go-systemd#352. But even
with these changes our tests couldn't build the test suits. Since we are using
`github.com/coreos/go-systemd/v22/util` which tries to include some os specific
binary files.

Solution is to exclude problematic modules on window build via comment tags
jujubot added a commit to juju/juju that referenced this pull request Feb 5, 2021
#12566

Fixes https://discourse.charmhub.io/t/building-juju-2-5-0-on-windows/549

## `coreos/go-systemd'
Not all of the parts of systemd is buildable on windows, there is currently
a PR open for it - coreos/go-systemd#352. But even
with these changes our tests couldn't build the test suits. Since we are using
`github.com/coreos/go-systemd/v22/util` which tries to include some os specific binary files.

Solution is to exclude problematic modules on window build via comment tags

## Checklist

 - [ ] Requires a [pylibjuju](https://github.com/juju/python-libjuju) change
 - [ ] Added [integration tests](https://github.com/juju/juju/tree/develop/tests) for the PR
 - [ ] Added or updated [doc.go](https://discourse.jujucharms.com/t/readme-in-packages/451) related to packages changed
 - [x] Comments answer the question of why design decisions were made
@lucab
Copy link
Contributor

lucab commented Feb 23, 2021

@ncruces can you please rebase on current master and squash your changes into a single commit?

These changes allow go-systemd to build on Windows.

Ideally, on platforms (like Windows) where systemd is not available,
go-systemd should behave similarly to how it does on Linux,
when a service is not started by systemd.

Tipically this is default, no-op, behavior.
@ncruces
Copy link
Contributor Author

ncruces commented Feb 23, 2021

Hi @lucab!
Is this what you meant?
Doing it in one commit unfortunately loses track of the rename.

@lucab lucab changed the title build on windows go-systemd: fix building on windows Feb 23, 2021
@lucab lucab changed the title go-systemd: fix building on windows Fix building on windows Feb 23, 2021
@lucab lucab merged commit 9e0bb8c into coreos:master Feb 23, 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.

None yet

2 participants