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 dragonfly #1224

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@mneumann
Copy link
Contributor

mneumann commented Sep 16, 2016

Compiles ponyc on DragonFlyBSD. Test suite runs except one test case.

@SeanTAllen

This comment has been minimized.

Copy link
Member

SeanTAllen commented Sep 16, 2016

@mneumann thanks for this!

@SeanTAllen

This comment has been minimized.

Copy link
Member

SeanTAllen commented Sep 16, 2016

@mneumann hopefully we will eventually have CI for BSD, at the moment, that isnt possible as we are relying on free services. Eventually, I hope we can get enough monetary support from the community to run our own BSD CI.

@SeanTAllen

This comment has been minimized.

Copy link
Member

SeanTAllen commented Sep 16, 2016

@sylvanc i feel like this raises the question of, could we do a better job of handling platforms supported rather than a "here's another value to add to if statements" approach we currently have.

@jemc

This comment has been minimized.

Copy link
Member

jemc commented Sep 16, 2016

@SeanTAllen - I think it's a good point to discuss

@mneumann - pardon my ignorance, but what are the practical differences for our purposes between freebsd and dragonfly? For the purposes of ifdefs in Pony, would it make sense to make dragonfly and freebsd synonymous, perhaps under the name bsd?

@@ -7,7 +7,7 @@

#if defined(PLATFORM_IS_LINUX)
# define ASIO_USE_EPOLL
#elif defined(PLATFORM_IS_MACOSX) || defined(PLATFORM_IS_FREEBSD)
#elif defined(PLATFORM_IS_MACOSX) || defined(PLATFORM_IS_FREEBSD) | defined(PLATFORM_IS_DRAGONFLY)

This comment has been minimized.

@mneumann

mneumann Sep 16, 2016

Contributor

needs to be || defined(PLATFORM_IS_DRAGONFLY)

@SeanTAllen

This comment has been minimized.

Copy link
Member

SeanTAllen commented Sep 17, 2016

When adding support for Dragonfly, I think we should also be updating the README install instructions to include Dragonfly in some fashion. Perhaps changing "FreeBSD" to "FreeBSD and DragonflyBSD".

Additionally, we would need to find any documentation on support platforms in this repo and update (as part of this PR) and also in any other repos.

@SeanTAllen

This comment has been minimized.

Copy link
Member

SeanTAllen commented Sep 17, 2016

Additionally, when we squash and merge this, I'd want to change from "Fix Dragonfly" to "Add support for DragonflyBSD"

Given that Dragonfly is currently unsupported, we should be clear that we are adding support for it and we should, as a team, recognize what that means in terms of commitment on our part for providing support, releases, etc.

@SeanTAllen

This comment has been minimized.

Copy link
Member

SeanTAllen commented Sep 17, 2016

@mneumann I'm not comfortable merging with a failing test case without knowing more details. Which test fails? What's the error?

@SeanTAllen

This comment has been minimized.

Copy link
Member

SeanTAllen commented Sep 18, 2016

This needs a CHANGELOG.md entry

@sylvanc

This comment has been minimized.

Copy link
Contributor

sylvanc commented Sep 21, 2016

The test is SugarTest.IfDefPosix that's failing. That should be an easy fix.

@sylvanc

This comment has been minimized.

Copy link
Contributor

sylvanc commented Sep 21, 2016

It looks like we could detect DragonFly in the Makefile and define PLATFORM_IS_FREEBSD internally. That would mean Platform.freebsd() would return true for DragonFly.

Is that an acceptable consequence for lowering the maintenance burden?

@mneumann

This comment has been minimized.

Copy link
Contributor

mneumann commented Sep 22, 2016

@SeanTAllen @sylvanc : Yes the failing test is SugarTest.IfDefPosix is as far as I understand somewhat related to how the macro expansion of the ifdefs work. I don't exactly understand how they work.

It think there would be all sorts of problems to declare DragonFly as FreeBSD. Also this would not help the other BSDs. Instead we should group common BSD-behaviour as such. For example with a PLATFORM_IS_BSD, which covers 90% of all BSDs.

@sylvanc

This comment has been minimized.

Copy link
Contributor

sylvanc commented Sep 28, 2016

@mneumann I think you are right about PLATFORM_IS_BSD being the way to go. I think we should go with that, and add Platform.bsd() as well. That will make future OpenBSD, NetBSD, etc ports easier to do.

Would you be up for refactoring this PR to do that? Someone can help you with the failing test case :) It's a bit tricky in there.

@SeanTAllen

This comment has been minimized.

Copy link
Member

SeanTAllen commented Sep 28, 2016

@mneumann i can work on finding someone who can help with the test failure if you take on the refactoring of the PR.

@SeanTAllen

This comment has been minimized.

Copy link
Member

SeanTAllen commented Oct 20, 2016

Marking this as discuss at sync as there has been no movement on this for a while.

We need to decide at sync if we want to take on support for DragonFly BSD as a "probably works" platform? And if we decide to, who will take over this PR and bring it home and exactly what "bring it home" means.

@SeanTAllen SeanTAllen force-pushed the ponylang:master branch 6 times, most recently from 97b1943 to 21d37aa Mar 11, 2017

@jemc jemc added stale and removed priority: 1 - low labels Jul 17, 2017

@SeanTAllen

This comment has been minimized.

Copy link
Member

SeanTAllen commented Aug 20, 2017

I'm closing this as I've opened #2183

@SeanTAllen SeanTAllen closed this Aug 20, 2017

@SeanTAllen SeanTAllen removed the stale label Aug 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment