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

Support FreeBSD 12 (64-bit inodes) #5199

Merged
merged 1 commit into from Jan 2, 2018
Merged

Conversation

valpackett
Copy link
Contributor

(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
Copy link

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

@valpackett
Copy link
Contributor Author

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
Copy link

Ah, great. Sorry for the noise then.

Copy link
Contributor

@RX14 RX14 left a comment

Choose a reason for hiding this comment

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

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

@ysbaddaden
Copy link
Contributor

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?

@valpackett
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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).

@valpackett
Copy link
Contributor Author

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

@RX14
Copy link
Contributor

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") %}
Copy link
Member

Choose a reason for hiding this comment

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

No need to use symbol, you can just write:

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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
lukeasrodgers pushed 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants