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

Doesn't build on FreeBSD #1645

Closed
hrefhref opened this issue Aug 12, 2021 · 21 comments · Fixed by #2004
Closed

Doesn't build on FreeBSD #1645

hrefhref opened this issue Aug 12, 2021 · 21 comments · Fixed by #2004

Comments

@hrefhref
Copy link

hrefhref commented Aug 12, 2021

Kratos (also affects Keto) does not build (gmake install) on FreeBSD:

GO111MODULE=on go install -tags sqlite .
# github.com/ory/dockertest/v3/docker/pkg/system
../../go/pkg/mod/github.com/ory/dockertest/[email protected]/docker/pkg/system/mknod.go:12:19: cannot use dev (type int) as type uint64 in argument to unix.Mknod
gmake: *** [Makefile:63: install] Error 2

Reproducing the bug

  1. Boot a FreeBSD.
  2. pkg install go sqlite3 bash gmake
  3. ln -s /usr/local/bin/bash /bin/bash (this is hardcoded in the Makefile)
  4. Clone Kratos
  5. gmake install

Environment

  • Git main: 11bdc4a
  • go1.16.7
  • FreeBSD 13.0-RELEASE
@aeneasr
Copy link
Member

aeneasr commented Sep 4, 2021

Unfortunately I don’t use FreeBSD and do not know how to resolve it. It looks oike a deep dependency within dockertest. I fear there’s nothing I can do on our end here, sorry!

If you know how to fix it, please supply a PR!

@hrefhref
Copy link
Author

hrefhref commented Sep 4, 2021 via email

@aeneasr
Copy link
Member

aeneasr commented Sep 5, 2021

It’s actually only used in test but Go is notorious for compiling everything, including tests, when building binaries :/

@hrefhref
Copy link
Author

hrefhref commented Sep 5, 2021

oof. So the only possible option is to fix dockertest compilation, even if there's zero chance of Docker working in freebsd ?

@aeneasr
Copy link
Member

aeneasr commented Sep 5, 2021

It's just the compilation which is broken, Docker is only needed to run tests - but yeah, the compile needs to be fixed first.

@joh-ku

This comment has been minimized.

@michael-o
Copy link

@ory seriuously? So there is nothing else around Linux for you? The world seems to be flat these days.

@NickUfer
Copy link
Contributor

NickUfer commented Nov 28, 2021

@michael-o I don't know if you get it but the problem was how your colleague communicated his points. Next time a bit less passive aggressive and more constructive should to the trick.

@michael-o
Copy link

@michael-o I don't know if you get it but the problem was how your colleague communicated his points. Next time a bit less passive aggressive and more constructive should to the trick.

He sharply pointed out that your approach is unnecessarily platform specific and violates your own goals. I have such comments many times taken as a personal attack although he's trying to tell you that the world isn't flat.

@aeneasr
Copy link
Member

aeneasr commented Nov 28, 2021

I overreacted (and am sorry for that) because I found the tone very rude and the points made are incorrect, clearly indicating that the poster has no idea how the software works. I have reverted the actions and am looking forward to useful and constructive ideas to resolve build issues on FreeBSD. Please be nice in open source projects, this project in particular is only maintained by me and I have little patience for trolls.

@michael-o
Copy link

I overreacted (and am sorry for that) because I found the tone very rude and the points made are incorrect, clearly indicating that the poster has no idea how the software works. I have reverted the actions and am looking forward to useful and constructive ideas to resolve build issues on FreeBSD. Please be nice in open source projects, this project in particular is only maintained by me and I have little patience for trolls.

Reacting properly to harsh criticism is a virtue. Hard to achieve, granted.

@michael-o
Copy link

michael-o commented Nov 28, 2021

There are two ways to look at it: You are doing a good job, but we can make it better OR you are doing a shifty job, why can't you do it right? I prefer to interpret @joh-ku's comment explicitly as the former.

Personally for me as a developer I feel great when I know that my code runs on many platforms identically and makes millions happy.
PS: I am biased because I do Maven releases and the above is true.

@joh-ku
Copy link

joh-ku commented Nov 28, 2021

@aeneasr The way you reacted to my comment is completely inappropriate and, in contrast to my (perhaps to harsh) technical input, it was without any doubt a rude attack against my person (I resist to post its content here). This is simply unprofessional, no matter if we are in OSS or commercial space or anywhere else.
If you felt in any way offended by my words I'd like to point out that this for sure was not my intention, so sorry for that.

But let's return to the content:

He sharply pointed out that your approach is unnecessarily platform specific and violates your own goals.

Exactly this is my point. This is not trolling. It's about portability. Nobody commented on this so far.

I overreacted (and am sorry for that) because I found the tone very rude and the points made are incorrect, clearly indicating that the poster has no idea how the software works.

May I ask you to explain why my points are incorrect?

@aeneasr
Copy link
Member

aeneasr commented Nov 28, 2021

The bug is most likely resolvable via:

  1. Identifying the dependency causing the build issue on FreeBSD
  2. Updating the dependency using go mod replace if there is a fix for the dependency available
  3. If not make a PR at the dependency resolving the issue
  4. If none of the above are an option, write a replacement for said dependency

As stated before, due to the way we bootstrap tests for easy consumption, it is not possible nor desirable to strip Dockertest from the project.

And as with all open source project, collaboration is key! If this is a problem you encounter, you’re in the best position to fix it! Maintainers including myself are happy to help wherever possible

@aeneasr
Copy link
Member

aeneasr commented Nov 28, 2021

If you felt in any way offended by my words I'd like to point out that this for sure was not my intention, so sorry for that.

Apology accepted, I hope you accept mine too :) My remarks were out of line and indeed unprofessional.

@joh-ku
Copy link

joh-ku commented Nov 28, 2021

Apology accepted, I hope you accept mine too :) My remarks were out of line and indeed unprofessional.

Accepted as well. :) I'm well aware that OSS maintenance is not an easy task and I highly appreciate your work.

I think, as already pointed out by others, the main problem is that FreeBSD does not support docker. If an encapsulation of business logic and deployment is not desired we probably are somewhat stuck at this point. I just think that this is not an ideal solution portability-wise like we can see from this issue (and therefore against my understanding of your design principles) – and please let me emphasize again, that I don't want to offend anybody with that statement. It's solely a technical note.

@aeneasr
Copy link
Member

aeneasr commented Nov 28, 2021

I see - so Docker ist not needed to run, it’s merely a tool to bootstrap tests! I think the problem is some incorrect syscall value in some dep that causes the build failure! :)

aeneasr added a commit to ory/dockertest that referenced this issue Nov 28, 2021
aeneasr added a commit that referenced this issue Nov 28, 2021
@michael-o
Copy link

Regardless of the fact that this is some Docker module:

../../go/pkg/mod/github.com/ory/dockertest/[email protected]/docker/pkg/system/mknod.go:12:19: cannot use dev (type int) as type uint64 in argument to unix.Mknod

and it should be skipped for non-Linux in most cases the underlying C/OS call isn't portable because the struct you are trying to read has either different types or members.

Definition of dev_t on FreeBSD: https://github.com/freebsd/freebsd-src/blob/373ffc62c158e52cde86a5b934ab4a51307f9f2e/sys/sys/_types.h#L113: uint64_t
Definiton of dev_t on Linux: https://elixir.bootlin.com/linux/latest/source/include/linux/types.h#L13: uint32_t.
Well, this isn't going to work.

Here: https://github.com/ory/dockertest/blob/44efd604a6887f5f132e2f076307f3a1ddf471ff/docker/pkg/system/mknod.go#L11-L13

You like need to take the largest available type here. I don't know how platform-specific code is done in Go, obviously not ifdefs.
BUT since this is dockertest it does not make any sense to port it since Docker a whole uses Linux-only features. It should be skipped in the build by OS detection.

Note: I have never written Go, but ported many OSS to FreeBSD and HP-UX, found many non-portable pitfalls down the road.

@aeneasr
Copy link
Member

aeneasr commented Nov 28, 2021

You're right :) All that was needed was to add a FreeBSD version of the mkdir file, which, when using freebsd was expecting an uint instead of an int as on other platforms (mac, windows, linux). So added a file to deal with the type conversion for freebsd, afaik building works now!

@michael-o
Copy link

michael-o commented Nov 28, 2021

You're right :) All that was needed was to add a FreeBSD version of the mkdir file, which, when using freebsd was expecting an uint instead of an int as on other platforms (mac, windows, linux). So added a file to deal with the type conversion for freebsd, afaik building works now!

Hopefully it won't suffer from any truncation because you pass in func Mknod and int. And Go's Mkdev will properly map to FreeBSD's makedev(int, int).

@joh-ku
Copy link

joh-ku commented Nov 28, 2021

Build works now! Thank you @aeneasr and @michael-o :)

The remaining problem with the Makefile (as bash defaults to /usr/local/bin/bash)

# GO111MODULE=on gmake install
gmake: /bin/bash: No such file or directory

can probably be fixed by replacing

SHELL=/bin/bash -o pipefail

with

SHELL=bash -o pipefail

as you export PATH anyway.

peturgeorgievv pushed a commit to senteca/kratos-fork that referenced this issue Jun 30, 2023
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 a pull request may close this issue.

5 participants