enable enc2 #65
Open
enable enc2 #65
+156
−25
Conversation
This file contains 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
Phantomical
added a commit
to Phantomical/xed-sys
that referenced
this pull request
Jan 16, 2024
It has been a few years since I have touched the xed-sys crate and advancements to the rust ecosystem have made some of the hacks we used to need obsolete. This commit reworks how and when bindings are generated. The new approach has two main advantages: - With no extra features enabled the build script doesn't even have to run bindgen at all. It can just use the bundled bindings directly. - We no longer have to maintain a separate c2rust translation of a subset of the functions exposed by XED. A new experimental feature in bindgen can now do this (mostly) automatically. I have also thrown in the enc2 feature that is in the process of being introduced by rust-xed#65. We do end up needing bindgen when the enc2 feature is enabled as the generated bindings would be too large to submit to crates.io. (They are >40MB!). With that said, here's how things work now: 1. Bindings for the default featureset are generated in advance by running `./generate.sh`. XED's headers don't contain any platform-dependent code so this should remain portable to any platforms where XED is supported. 2. When the bindgen feature is enabled we ignore the bindings from step 1 and instead generate them in build.rs. With the new experimental --wrap-static-fns bindgen option we can drop the janky c2rust step that translates versions of these functions to rust so that they can be included. Instead bindgen generates an additional C file that declares a set of new functions that forward to the static functions, the generated bindings refer to these forwarding functions instead of the original ones.
Phantomical
added a commit
to Phantomical/xed-sys
that referenced
this pull request
Jan 16, 2024
It has been a few years since I have touched the xed-sys crate and advancements to the rust ecosystem have made some of the hacks we used to need obsolete. This commit reworks how and when bindings are generated. The new approach has two main advantages: - With no extra features enabled the build script doesn't even have to run bindgen at all. It can just use the bundled bindings directly. - We no longer have to maintain a separate c2rust translation of a subset of the functions exposed by XED. A new experimental feature in bindgen can now do this (mostly) automatically. I have also thrown in the enc2 feature that is in the process of being introduced by rust-xed#65. We do end up needing bindgen when the enc2 feature is enabled as the generated bindings would be too large to submit to crates.io. (They are >40MB!). With that said, here's how things work now: 1. Bindings for the default featureset are generated in advance by running `./generate.sh`. XED's headers don't contain any platform-dependent code so this should remain portable to any platforms where XED is supported. 2. When the bindgen feature is enabled we ignore the bindings from step 1 and instead generate them in build.rs. With the new experimental --wrap-static-fns bindgen option we can drop the janky c2rust step that translates versions of these functions to rust so that they can be included. Instead bindgen generates an additional C file that declares a set of new functions that forward to the static functions, the generated bindings refer to these forwarding functions instead of the original ones.
Phantomical
added a commit
that referenced
this pull request
Jan 16, 2024
It has been a few years since I have touched the xed-sys crate and advancements to the rust ecosystem have made some of the hacks we used to need obsolete. This commit reworks how and when bindings are generated. The new approach has two main advantages: - With no extra features enabled the build script doesn't even have to run bindgen at all. It can just use the bundled bindings directly. - We no longer have to maintain a separate c2rust translation of a subset of the functions exposed by XED. A new experimental feature in bindgen can now do this (mostly) automatically. I have also thrown in the enc2 feature that is in the process of being introduced by #65. We do end up needing bindgen when the enc2 feature is enabled as the generated bindings would be too large to submit to crates.io. (They are >40MB!). With that said, here's how things work now: 1. Bindings for the default featureset are generated in advance by running `./generate.sh`. XED's headers don't contain any platform-dependent code so this should remain portable to any platforms where XED is supported. 2. When the bindgen feature is enabled we ignore the bindings from step 1 and instead generate them in build.rs. With the new experimental --wrap-static-fns bindgen option we can drop the janky c2rust step that translates versions of these functions to rust so that they can be included. Instead bindgen generates an additional C file that declares a set of new functions that forward to the static functions, the generated bindings refer to these forwarding functions instead of the original ones.
|
See the comment below. |
|
So, as it turns out, the two libraries that are produced by the xed build system when you build with I'm not entirely sure what the solution for this is. As is, I've reopened the PR and updated it to work against the current master. The real solution for this looks like it should be somehow linking both within the xed build system, but that seems like it would be hard to do. I'm going to leave this open for now in case someone wants to figure out a solution. Detailed link error message |
Maybe we could split it to
agree, |
When linking statically it is not possible to link both enc2 libraries. However, once they are compiled as dynamic libraries then linking both just works.
|
I have played around with this PR a bit and I think I've got it to mostly work.
Normally I would be rather hesitant about this because these features are not additive. If two of your dependencies end up depending on both With that said, it turns out that if you link both of the enc2 libraries as shared libraries then linking works just fine! This means that there is a way out for dependencies - even if it is a little bit inconvenient. xed-sys isn't used enough for this to be a big issue so I'm ok with this as-is. Now the one last thing I need you to do before I would be confident in merging this is writing some tests that use the |
You need this in order for rustc to correctly set the path so that includes the xed DLLs.
There is link issue when dylib and the enc2-chk libraries are linked that results in missing symbol warnings. This doesn't seem like something to fix in the current PR
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
add feature to enable enc2 encoder, and disable it by default because it is huge size