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

Support FreeBSD 12 (64-bit inodes) #5199

Merged
merged 1 commit into from Jan 2, 2018

Conversation

Projects
None yet
6 participants
@myfreeweb
Contributor

myfreeweb commented Oct 28, 2017

(also we're unknown now instead of portbld, so I added a symlink there)

THANK YOU for simply splitting the llvm target on - and shoving everything into flags. Rust currently drops the version number from targets and it's painful.

// Ideally, this would use a version comparison >= 12.0, but this is fine for now.

@ararslan

This comment has been minimized.

Show comment
Hide comment
@ararslan

ararslan Nov 10, 2017

Very exciting! Would it be possible to enable 11.1 or is there something specific to current that's required?

ararslan commented Nov 10, 2017

Very exciting! Would it be possible to enable 11.1 or is there something specific to current that's required?

@myfreeweb

This comment has been minimized.

Show comment
Hide comment
@myfreeweb

myfreeweb Nov 10, 2017

Contributor

11.1 worked as is. On 12 it was broken because of the inode/dirent/stat changes in the kernel. This fix uses the new structs on 12.

Contributor

myfreeweb commented Nov 10, 2017

11.1 worked as is. On 12 it was broken because of the inode/dirent/stat changes in the kernel. This fix uses the new structs on 12.

@ararslan

This comment has been minimized.

Show comment
Hide comment
@ararslan

ararslan Nov 10, 2017

Ah, great. Sorry for the noise then.

ararslan commented Nov 10, 2017

Ah, great. Sorry for the noise then.

@RX14

RX14 approved these changes Nov 12, 2017

This is fine for now, but we should probably think a bit more about how to handle the BSDs' libc in the future.

@ysbaddaden

This comment has been minimized.

Show comment
Hide comment
@ysbaddaden

ysbaddaden Nov 13, 2017

Member

FreeBSD 12.0 is -CURRENT hence not released, so there is no need to rush and merge.

C bindings should be considered as automatically generated. Starting to put flags to support one or another version is unexpected. Also, using freebsd12.0 will break on point releases (12.1) and next major releases (13.0).

Maybe we can introduce flags without the leading .x to target versions more broadly —I don't except point releases to break ABI... but maybe I'm wrong? Anyway we should reverse to check for freebsd10 and freebsd11 (supported versions) to rely on legacy structs, but use the new structs otherwise?

Member

ysbaddaden commented Nov 13, 2017

FreeBSD 12.0 is -CURRENT hence not released, so there is no need to rush and merge.

C bindings should be considered as automatically generated. Starting to put flags to support one or another version is unexpected. Also, using freebsd12.0 will break on point releases (12.1) and next major releases (13.0).

Maybe we can introduce flags without the leading .x to target versions more broadly —I don't except point releases to break ABI... but maybe I'm wrong? Anyway we should reverse to check for freebsd10 and freebsd11 (supported versions) to rely on legacy structs, but use the new structs otherwise?

@myfreeweb

This comment has been minimized.

Show comment
Hide comment
@myfreeweb

myfreeweb Nov 13, 2017

Contributor

Yeah, that's why I said

Ideally, this would use a version comparison >= 12.0

I guess I should work on that…

Point releases don't break ABI, you're not wrong.

Contributor

myfreeweb commented Nov 13, 2017

Yeah, that's why I said

Ideally, this would use a version comparison >= 12.0

I guess I should work on that…

Point releases don't break ABI, you're not wrong.

@RX14

This comment has been minimized.

Show comment
Hide comment
@RX14

RX14 Nov 13, 2017

Member

@ysbaddaden you can't consider the C bindings as automatically generated unless they are automatically generated. I never got the generator to work and contributors are unlikely to do the same, unless the whole generation process comes down to a simple, fairly portable command. I doubt many of the recent lib_c changes were not manually done.

Member

RX14 commented Nov 13, 2017

@ysbaddaden you can't consider the C bindings as automatically generated unless they are automatically generated. I never got the generator to work and contributors are unlikely to do the same, unless the whole generation process comes down to a simple, fairly portable command. I doubt many of the recent lib_c changes were not manually done.

@ysbaddaden

This comment has been minimized.

Show comment
Hide comment
@ysbaddaden

ysbaddaden Nov 14, 2017

Member

I never said we can't maintain C bindings manually, but to keep in mind they should be auto-generated and will eventually be (since we can't rely on fixed sets, as this issue outlines) to help avoid changes that would prevent this.

I don't understand how nobody got the generator working. Yes, cross compiling headers for a foreign platform is tricky (because local compiler & system headers get in the way), but generating bindings for the current platform is a mere shards install ; make crystal (ok, it broke with crystal 0.24 because of yaml changes).

Member

ysbaddaden commented Nov 14, 2017

I never said we can't maintain C bindings manually, but to keep in mind they should be auto-generated and will eventually be (since we can't rely on fixed sets, as this issue outlines) to help avoid changes that would prevent this.

I don't understand how nobody got the generator working. Yes, cross compiling headers for a foreign platform is tricky (because local compiler & system headers get in the way), but generating bindings for the current platform is a mere shards install ; make crystal (ok, it broke with crystal 0.24 because of yaml changes).

@myfreeweb

This comment has been minimized.

Show comment
Hide comment
@myfreeweb

myfreeweb Nov 14, 2017

Contributor

This issue also means that autogeneration would only generate code for one OS version, unless you make it read headers from multiple versions.

Contributor

myfreeweb commented Nov 14, 2017

This issue also means that autogeneration would only generate code for one OS version, unless you make it read headers from multiple versions.

@RX14

This comment has been minimized.

Show comment
Hide comment
@RX14

RX14 Nov 14, 2017

Member

@ysbaddaden the benefits of automation here are consistency and reproducibility. As people have demonstrated with their PRs, doing it manually is easier than using posix bindgen so ease of use certainly is not what was achieved. As soon as you edit it by hand all of those benefits are lost and to me it's as good as manually generated. Unless there's a single command to generate these bindings for all platforms we support automatically and they're actually kept in sync, all posix is good for was the chore of writing lib_c in the first place.

Member

RX14 commented Nov 14, 2017

@ysbaddaden the benefits of automation here are consistency and reproducibility. As people have demonstrated with their PRs, doing it manually is easier than using posix bindgen so ease of use certainly is not what was achieved. As soon as you edit it by hand all of those benefits are lost and to me it's as good as manually generated. Unless there's a single command to generate these bindings for all platforms we support automatically and they're actually kept in sync, all posix is good for was the chore of writing lib_c in the first place.

@@ -9,9 +9,17 @@ lib LibC
alias DevT = UInt
alias GidT = UInt
alias IdT = Long
alias InoT = UInt
{% if flag?(:"freebsd12.0") %}

This comment has been minimized.

@asterite

asterite Jan 2, 2018

Contributor

No need to use symbol, you can just write:

{% if flag?("freebsd12.0") %}
@asterite

asterite Jan 2, 2018

Contributor

No need to use symbol, you can just write:

{% if flag?("freebsd12.0") %}

This comment has been minimized.

@straight-shoota

straight-shoota Jan 2, 2018

Contributor

Hm, now this was merged with symbol...?

@straight-shoota

straight-shoota Jan 2, 2018

Contributor

Hm, now this was merged with symbol...?

This comment has been minimized.

@asterite

asterite Jan 2, 2018

Contributor

It was just a note, not super necessary to make it work. There's also no difference regarding performance or anything, just style.

@asterite

asterite Jan 2, 2018

Contributor

It was just a note, not super necessary to make it work. There's also no difference regarding performance or anything, just style.

@RX14 RX14 added this to the Next milestone Jan 2, 2018

@RX14 RX14 merged commit a3f2ecc into crystal-lang:master Jan 2, 2018

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

lukeasrodgers added a commit to lukeasrodgers/crystal that referenced this pull request Jan 7, 2018

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