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

DragonFly BSD 4.8 support #2183

Merged
merged 30 commits into from
Aug 24, 2017
Merged

DragonFly BSD 4.8 support #2183

merged 30 commits into from
Aug 24, 2017

Conversation

SeanTAllen
Copy link
Member

@SeanTAllen SeanTAllen commented Aug 20, 2017

I got bored. I did this.

In many ways, FreeBSD and DragonFly are the same in that they are BSD based. However, they are entirely different OSes in a way that Debian and Arch are not. From a "platform" support perspective, I ended up going with defining bsd/freebsd/dragonfly. I'm not sure this is the correct long term thing. I'd want to implement OpenBSD and/or NetBSD before drawing any conclusions.

I'm opening this to get feedback on where this is at so far.

@SeanTAllen SeanTAllen added the do not merge This PR should not be merged at this time label Aug 20, 2017
# include <stdalign.h>
#endif

#include <platform.h>
Copy link
Member Author

Choose a reason for hiding this comment

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

this is needed because dragonfly uses GCC but it doesn't use glibc so there's not <features.h> as referenced below. perhaps there is a better way to do this?

Copy link
Member

Choose a reason for hiding this comment

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

The problem here is that platform.h is a private header while atomics.h is a public one. Depending on how much of platform.h is needed, I'd suggest to either copy/paste the relevant parts into atomics.h or to make a new platform_public.h file which would be included by both platform.h and atomics.h, and installed alongside of pony.h and atomics.h.

Copy link
Member

@Praetonus Praetonus Aug 20, 2017

Choose a reason for hiding this comment

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

In fact I think we don't even need to include features.h anymore. This was needed for the __GNUC_PREREQ macro but we stopped using it in the Alpine port.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Praetonus let me try removing entirely and lets see what happens.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch Benoit. We didn't need features.h anymore. I've removed it entirely along with the platform.h inclusion.

" $extra $ifdefnot $noseq($flag linux $ifdefor $flag osx $ifdefor\n"
" $flag freebsd) then\n"
" $flag bsd) then\n"
Copy link
Member Author

@SeanTAllen SeanTAllen Aug 20, 2017

Choose a reason for hiding this comment

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

should freebsd and dragonfly be in this as well? i dont think so...

@SeanTAllen SeanTAllen mentioned this pull request Aug 20, 2017
@SeanTAllen
Copy link
Member Author

@Theodus can you check to make sure this still builds and passes on FreeBSD?

@SeanTAllen SeanTAllen requested a review from Theodus August 20, 2017 13:54
Copy link
Contributor

@Theodus Theodus left a comment

Choose a reason for hiding this comment

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

Works on FreeBSD!

Benoit says we no longer need.
@SeanTAllen
Copy link
Member Author

thanks @Theodus. when this is in "final shape", i'm going to ask you for one last test.

Makefile Outdated
@@ -168,6 +173,10 @@ ifeq ($(OSTYPE),osx)
ALL_CXXFLAGS += -stdlib=libc++ -mmacosx-version-min=10.8
endif

ifeq ($(UNAME_S),DragonFly)
LINKER_FLAGS += -L /usr/local/lib
Copy link
Member Author

Choose a reason for hiding this comment

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

/usr/local/lib is explicitly required for DragonFly.

Is there any reason to not make this a default path for every OS?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds right to me (at least, for every non-Windows OS 😉).

@SeanTAllen
Copy link
Member Author

This is now complete and will build on DragonFly. I'd be good with squashing this down if there are no changes and getting it on master. I'm leaving for feedback at sync (I do have a couple open questions) and then one final whirl from @Theodus on FreeBSD.

@SeanTAllen SeanTAllen added the changelog - added Automatically add "Added" CHANGELOG entry on merge label Aug 22, 2017
@SeanTAllen SeanTAllen changed the title Dragonfly 4_8 support Dragonfly 4.8 support Aug 22, 2017
@SeanTAllen SeanTAllen changed the title Dragonfly 4.8 support DragonFly BSD 4.8 support Aug 22, 2017
@Theodus
Copy link
Contributor

Theodus commented Aug 22, 2017

Works on FreeBSD 11.0-RELEASE-p1

@SeanTAllen
Copy link
Member Author

I ll make the Makfile change and then squash this.

@SeanTAllen SeanTAllen merged commit d59c185 into master Aug 24, 2017
ponylang-main added a commit that referenced this pull request Aug 24, 2017
@SeanTAllen SeanTAllen deleted the dragonfly_48 branch August 24, 2017 16:15
winksaville added a commit to winksaville/ponyc that referenced this pull request Mar 3, 2018
winksaville added a commit to winksaville/ponyc that referenced this pull request Mar 3, 2018
winksaville added a commit to winksaville/ponyc that referenced this pull request Mar 7, 2018
These changes serve no real purpose in gbenchmark where there are no
differences between FREE BSD and DragonFly BSD.

I didn't realize that when I created PR ponylang#2573 "Update gbenchmark" and
merged in the gbenchmark changes from PR ponylang#2183. This reverts back to
just using BENCHMARK_OS_FREEBSD in lib/gbenchmark and thus a few less
differences between lib/gbenchmark and google/benchmark.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - added Automatically add "Added" CHANGELOG entry on merge do not merge This PR should not be merged at this time
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants