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

Add support for Solaris #470

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

vedranmiletic
Copy link
Contributor

Tested on illumos distribution OmniOS Community Edition.

Not sure if we care about this (I don't use Solaris in production), but I wanted to learn more about portability so I did the porting.

@vedranmiletic
Copy link
Contributor Author

It requires an-tao/trantor#94 for the tests to pass. I also verified that the tests still pass on Linux and FreeBSD after these changes.

@an-tao an-tao requested a review from rbugajewski June 9, 2020 15:35
Copy link
Member

@an-tao an-tao left a comment

Choose a reason for hiding this comment

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

@vedranmiletic the drogon_ctl -v command doesn't work after this PR, please check it. I guess you renamed the version class to drogonVersion for avoiding name conflict, please use namespace for this. And please try to keep drogon_ctl help command consistent with before.

@rbugajewski
Copy link
Collaborator

I’ll make some sanity checks on macOS tomorrow.

@vedranmiletic
Copy link
Contributor Author

@vedranmiletic the drogon_ctl -v command doesn't work after this PR, please check it. I guess you renamed the version class to drogonVersion for avoiding name conflict, please use namespace for this. And please try to keep drogon_ctl help command consistent with before.

You're indeed correct, I should have been more careful and checked drogon_ctl alongside tests. Anyway, I fixed it with a namespace as you suggested.

@vedranmiletic
Copy link
Contributor Author

I have no idea why PostgreSQL failed in Travis on Linux. Any pointers are welcome.

@vedranmiletic
Copy link
Contributor Author

vedranmiletic commented Jun 9, 2020

Wait, I figured now I should also run test.sh locally. Luckily, on Linux and FreeBSD it still works after applying this patch, but note that I don't have PostgreSQL and MySQL set up for testing.

However, on Solaris webapp_test blocks indefinitely on a future get() and WebSocket test assert fails. So give me a few days to investigate if this is easily fixable.

Do you maybe want to merge this as "Fix building and tests on Solaris" and then I can make a separate patch for the rest?

@an-tao
Copy link
Member

an-tao commented Jun 10, 2020

@vedranmiletic When a patch fails the existing test, I prefer not to merge it. I think you can create a port/solaris branch on this repository (instead of on your forked repo), you now have this permission.
Thanks for your work. I'll help if I can.

@vedranmiletic
Copy link
Contributor Author

@vedranmiletic When a patch fails the existing test, I prefer not to merge it. I think you can create a port/solaris branch on this repository (instead of on your forked repo), you now have this permission.

Makes sense. I don't have time today to resume working on this, but I will later this week. I first need to figure out why get() hangs, hopefully it's something simple. This is a good chance to understand how various parts of Drogon work together.

Thanks for your work. I'll help if I can.

You are very welcome to try installing OpenIndiana in VirtualBox or VMware. IIRC it had some issue with weird keyboard keys being repeatedly pressed when running under KVM/QEMU.

@an-tao
Copy link
Member

an-tao commented Jun 10, 2020

@vedranmiletic I compiled and tested drogon on OmniOS, and I found that errors are in the trantor library. The reading and writing on a TCP connection don't work. I'll try to figure it out.

@an-tao
Copy link
Member

an-tao commented Jun 10, 2020

@vedranmiletic I know what's wrong with trantor now. The IO multiplexer (epoll on linux and kqueue on macOS or FreeBSD) is different on solaris, the change you made just implement a kqueue poller without any functionality, so it can't work.

It seems that poll() is used on solaris, you could find the wapper of poll() in very early version of trantor (before kqueue was introdued into trantor), but I don't know if it is compatible with solaris. poll() on linux is slower than epoll(), I don’t know if it is also inefficient on solaris. I'm afraid there is a lot of work to be done for solaris.

@vedranmiletic
Copy link
Contributor Author

@an-tao Indeed, in trantor/net/inner/Poller.cc there's a piece of code:

#if defined __linux__ || defined _WIN32
    return new EpollPoller(loop);
#else
    return new KQueue(loop);
#endif

OK, I'll see what I can do.

@an-tao
Copy link
Member

an-tao commented Jun 10, 2020

@vedranmiletic

commit 49c6311642f38070d34dd1a4f0408f8f41a5ebfc
Merge: 4dade42 c611b9f
Author: an-tao <[email protected]>
Date:   Wed Dec 19 13:50:17 2018 +0800

    Merge pull request #5 from an-tao/kqueue
    
    Use kqueue instead of poll in MacOS/BSD

commit c611b9fc86d8894d2254694926c0354f0f1769e1
Author: antao <[email protected]>
Date:   Wed Dec 19 13:45:23 2018 +0800

    Use kqueue instead of poll in MacOS/BSD

commit 4dade42572b5fc053b4dbe21eede15d5b0f55197

see the git log, Following are the files in the trantor/net/inner/poller folder of the 4dade42 commit:

4805  6 11 00:16 EpollPoller.cc
  843  6 11 00:16 EpollPoller.h
4198  6 11 00:16 PollPoller.cc
1058  6 11 00:16 PollPoller.h

you could checkout the deleted PollPoller.cc from old version of trantor, and then:

#if defined __linux__ || defined _WIN32
    return new EpollPoller(loop);
#elsdef __sun
    return new PollPoller(loop);
#else
    return new KQueue(loop);
#endif

@vedranmiletic
Copy link
Contributor Author

@vedranmiletic

commit 49c6311642f38070d34dd1a4f0408f8f41a5ebfc
Merge: 4dade42 c611b9f
Author: an-tao <[email protected]>
Date:   Wed Dec 19 13:50:17 2018 +0800

    Merge pull request #5 from an-tao/kqueue
    
    Use kqueue instead of poll in MacOS/BSD

commit c611b9fc86d8894d2254694926c0354f0f1769e1
Author: antao <[email protected]>
Date:   Wed Dec 19 13:45:23 2018 +0800

    Use kqueue instead of poll in MacOS/BSD

commit 4dade42572b5fc053b4dbe21eede15d5b0f55197

see the git log, Following are the files in the trantor/net/inner/poller folder of the 4dade42 commit:

4805  6 11 00:16 EpollPoller.cc
  843  6 11 00:16 EpollPoller.h
4198  6 11 00:16 PollPoller.cc
1058  6 11 00:16 PollPoller.h

you could checkout the deleted PollPoller.cc from old version of trantor, and then:

#if defined __linux__ || defined _WIN32
    return new EpollPoller(loop);
#elsdef __sun
    return new PollPoller(loop);
#else
    return new KQueue(loop);
#endif

Thanks, was just looking for that commit. Exactly the approach I had in mind.

Tested on illumos distribution OmniOS Community Edition.
@vedranmiletic
Copy link
Contributor Author

Now that PollPoller is back in, let's see if I can get this working.

@an-tao
Copy link
Member

an-tao commented Aug 27, 2022

@vedranmiletic Thanks so much for resuming this PR, I have no environment of solaris OS, would you please test this PR on your Solaris host?

@vedranmiletic
Copy link
Contributor Author

Yes, sure. I'll get back with the results in a few days.

@an-tao
Copy link
Member

an-tao commented Sep 21, 2022

@vedranmiletic have you tested this? thanks.

@vedranmiletic
Copy link
Contributor Author

@an-tao I tried. Unfortunately, the machine that I could dedicate to Solaris isn't supported in terms of hardware. Virtual machines had trouble in the past. I will find time in the next few weeks to try on a server that worked well in the past and fix the issues that appear (if any).

@an-tao
Copy link
Member

an-tao commented Sep 27, 2022

@an-tao I tried. Unfortunately, the machine that I could dedicate to Solaris isn't supported in terms of hardware. Virtual machines had trouble in the past. I will find time in the next few weeks to try on a server that worked well in the past and fix the issues that appear (if any).

Thanks so much for your help!

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.

3 participants