-
Notifications
You must be signed in to change notification settings - Fork 30
Add Windows support #108
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Merged
bluejekyll
merged 4 commits into
bluejekyll:master
from
KeyrockEU:feature/add-windows-support
Dec 23, 2019
Merged
Add Windows support #108
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,8 @@ | ||
[build] | ||
# Informs the linker that some of the symbols for Postgres won't be available | ||
# until runtime on the dynamic library load. | ||
# until runtime on the dynamic library load. Flags differ based on target family | ||
|
||
[target.'cfg(unix)'] | ||
rustflags = "-C link-arg=-undefineddynamic_lookup" | ||
|
||
[target.'cfg(windows)'] | ||
rustflags = "-C link-arg=/FORCE" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whitelist can limit the usefulness of
pg-extend-rs
; some extension modules usepg_sys
to get access to additional PostgreSQL functions without needing to run bindgen themself.Have you considered any alternatives to such whitelisting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it can limit the usefulness, but it generates a HUGE file if you leave as default everything:
I'm not confortable with having a hard-coded list, that's why I limited only for Windows while an alternative is figured out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we introduce a feature flag that would enable all symbols vs. limit them?
“all-symbols” seems fairly obvious. How we dice and slice the smaller sets based on which features you’re enabling is a little harder to see.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem I encountered was that "allowing all" introuced a lot of things (if I'm not wrong, it imports the full Windows API) which I started by blacklisting them, but the list is so big, that it's simpler to do a whitelisting (which works recursively) of what was used from the crate.
As I mentioned, this whitelisting based approach is only for Windows. I left the "include all" approach for the
unix
familyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open to suggestions on how to tackle the white/black listing. One option would be to be able to pull the list of all the PG symbols and use it for the whitelisting, but I have no idea on how to do that or where to pull it from. If understand correctly how
bindgen
works, whitelisting that list of symbols will recursively include any non-PG symbols that the PG ones useThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if it does include the entirety of Windows API headers, what problems does that cause? Looking at bindgen's issue tracker, it seems that's a common way to use it. (e.g. rust-lang/rust-bindgen#1562 "I'm pretty sure on Firefox bindgen ends up including windows.h without issues")
You said it's slow to compile; is that the only reason why you want whitelisting? How much time does it save?
If it's merely an optimization of build speed, I don't see why this needs to be Windows-specific at all. Omitting potentially useful symbols from a module depending on platform, for only build speed reasons, seems like very surprising behavior to me, likely to cause porting issues to downstream users.
If the speed gains are worthwhile, a feature flag, as suggested by @bluejekyll, sounds like a great idea. Then Unix builds could also take advantage of it, if desired. But that would better be done as a separate PR IMO, and strip whitelisting from this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment @intgr. The reason to not include all was not due to slow compilation, but because it includes problematic code that has issues at the Rust compiler level.
I first tried the blacklisting approach but it was a difficult one so I moved to the whitelisting approach.
On the other hand (based on what I understood from @bluejekyll's comment, please correct me if I'm wrong), the list of whitelisted symbols could be based on feature flags, but I would still aim for the "include all" to be to whitelist all PG related things, which will recursivelly include any extra things that are required.
The fact that there's a ~130k LoC reduction was just a bonus, not the main reason for the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this issue
would be solved by updating bindgen. Our
Cargo.lock
file references bindgen 0.48.1 which is more than half a year old!The other issue might be solved as well (rust-lang/rust-bindgen#1498?). Give it a shot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I bumped
bindgen
to 0.51.1 and still the same errors