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

Add a vendored feature that uses a vendored copy #4

Closed
wants to merge 17 commits into from

Conversation

tanriol
Copy link
Owner

@tanriol tanriol commented Apr 13, 2020

Note that this feature requires splitting the crate in two to separate permissively licensed bindings and LGPL sources so that the crate licenses correctly match the content.

tanriol added 16 commits April 3, 2020 03:10
The `probe` function was introduced to replace `find` in version 0.3.7.
Make the pregenerated and compile-time cases more alike.
This avoids problems with linker that keeping C code and Rust bindings
to it causes.
This makes it possible to drop the pkg-config hack as it turns out to be
needed only for unit tests.
Not fully tested, but at least the build script seems to compile.
The libftdi1 API is defined in a cross-platform way and should just work
on new platforms. This feature may still be desired for cases like
building against a newer libftdi version.
@tanriol
Copy link
Owner Author

tanriol commented Apr 13, 2020

@cr1901 / @roqvist, would be cool if one of you could test whether everything still works on Windows as I don't have the corresponding development environment at hand. By "everything" here I mean all the supported feature configurations: none, vendored, bindgen. The combination of bindgen and vendored does not work (as in "bindgen is compiled, but not used"). Ideally one should test both that examples do compile and that tests do pass.

@tanriol
Copy link
Owner Author

tanriol commented Apr 13, 2020

Alternatively I'm going to make a few more textual cleanups and merge this anyway with the intention of adding CI later.

@cr1901
Copy link
Contributor

cr1901 commented Apr 13, 2020

@tanriol I'll test this within the next few hours. Yesterday, I was trying to create a draft for unifying safe-ftdi and ftdi-rs, but I went very deep down a Rust rabbit hole re: Trait objects and lifetime bounds. That derailed everything last night :P.

@cr1901
Copy link
Contributor

cr1901 commented Apr 14, 2020

@tanriol My guess is that with the common directory, you intended to create symlinks where each crate links to shared files. However, these symlinks don't work properly on my machine (even though I do have symlinks enabled). It's also no guarantee that a Windows user will have symlinks enabled:

William@DESKTOP-H0PMN4M MINGW64 ~/Projects/FPGA/libftdi1-sys/sys
$ cargo build
   Compiling libftdi1-sys v1.0.0-alpha1 (C:\msys64\home\William\Projects\FPGA\libftdi1-sys\sys)
error: expected item, found `..`
 --> sys\src\pregenerated.rs:1:1
  |
1 | ../../common/src/pregenerated.rs
  | ^^

error: aborting due to previous error

error: could not compile `libftdi1-sys`.

To learn more, run the command again with --verbose.

Additionally, if you're choosing not to publish the sys-vendored-lgpl crate (publish=false), how will someone build libftdi1-sys with the vendor feature enabled be able to download the crate? Is the entire workspace published for each crate every time you publish a crate in a workspace?

@tanriol
Copy link
Owner Author

tanriol commented Apr 14, 2020

...if you're choosing not to publish the sys-vendored-lgpl crate (publish=false)...

This is just a temporary setting - I don't have much experience with cargo publish and want to avoid accidentally publishing a non-working state. It will be removed before publishing.

However, these symlinks don't work properly on my machine (even though I do have symlinks enabled).

That's one of the problems I was afraid of. Any other ideas how to avoid duplication? Sure, we can just copy files, but that feels a bit wrong...

@cr1901
Copy link
Contributor

cr1901 commented Apr 14, 2020

That's one of the problems I was afraid of. Any other ideas how to avoid duplication? Sure, we can just copy files, but that feels a bit wrong...

I don't see how we get around copying files. However, to still get the benefits of code reuse, we could use build.rs to copy the relevant rust code to its final directories, and then include! them, just like we do with the bindgen bindings:

cfg_if::cfg_if! {
    if #[cfg(feature = "vendored")] {
        pub use libftdi1_sys_vendored_lgpl::*;
    } else if #[cfg(feature = "bindgen")] {
        include!(concat!(env!("OUT_DIR"), "/bindings.rs"));
    } else {
        include!("pregenerated.rs");
    }
}

I haven't fully thought out this idea yet, but it could be completely feasible to make Rust think the tests::shared module exists even if the shared files are copied to a single OUT_DIR without hierarchy.

@tanriol
Copy link
Owner Author

tanriol commented Apr 15, 2020

Ok, so I've thought about this and I don't see a good way to do this. The options I see are:

  1. Move the shared files into a separate crate, write them out in the build scripts. Advantages: nice layout. Disadvantages: a whole internal crate just for sharing two files?

  2. Layout directories like this: (main crate) -> (lgpl crate) -> (shared files) and play with includes. Advantages: just works. Disadvantages: "why are shared files in the lgpl dir?"

  3. Layout directories like this: (lgpl crate) -> (main crate) -> (shared files) and play with includes. Advantages: just works. Disadvantages: "why does the root contain the crate one should not use?"

  4. Just duplicate files between crates and write in the README that the contributor must check they are the same. Advantages: mostly nice layout. Disadvantages: requires attention from contributors whenever they do anything.

  5. Do nothing, just tell Windows contributors in the README that they should be in developer mode and have symlinks enabled in git (or clone with git clone -c core.symlinks=true https://...).

Your opinions?

P.S. Would be nice if you could check the suggested clone command - for me just git clone worked in a VM, but I noticed during Git for Windows installation that there was an on-by-default "enable symlinks" option in the installer.

@cr1901
Copy link
Contributor

cr1901 commented Apr 15, 2020

git clone -b feature-vendored -c core.symlinks=true https://github.com/tanriol/libftdi1-sys.git ftdi-test does work on my machine- i.e. it creates symlinks.

Seems up in the air how well cargo supports them though? rust-lang/cargo#5664

@tanriol
Copy link
Owner Author

tanriol commented Apr 15, 2020

P.P.S. Also looks like the vendored feature does not actually work in Windows because vcpkg does not provide correct include path and/or CFLAGS for libusb. On the other hand, probably on Windows it should cause libusb to be vendored as well.

@cr1901
Copy link
Contributor

cr1901 commented Apr 15, 2020

On the other hand, probably on Windows it should cause libusb to be vendored as well.

That is beyond the scope of what a vendored feature should do, IMHO. My policy w/ dealing w/ enough Windoze woes over the years is "I can't support every conceivable build configuration for Windows", so I limit to cases that have the most impact. pkg-config and vcpkg are probably the highest-impact solutions for now.

With that said, if a user is going to build using a local copy of libftdi, they should have prerequisite packages ahead of time. I would have to see the exact error you're getting to diagnose further; I don't see any indication as to whether vcpkg libftdi links to a static or dynamic libusb, only that it's a build dependency.

@tanriol
Copy link
Owner Author

tanriol commented Apr 15, 2020

Vcpkg libftdi seems to be fine. On the other hand, vendored libtfdi fails to build against vcpkg libusb1 with "cannot open include file 'libusb.h'" because the search path is (vcpkg installed root)/include/ while the header is in (vcpkg install root)/include/libusb-1.0/.

@tanriol
Copy link
Owner Author

tanriol commented Apr 15, 2020

Anyway, I'm getting tired of this problem, so, unless we can make good progress in the next day or two, I'll just release 1.0.0 without the vendored feature and postpone this problem till someone actually needs it.

@cr1901
Copy link
Contributor

cr1901 commented Apr 15, 2020

On the other hand, vendored libtfdi fails to build against vcpkg libusb1 with "cannot open include file 'libusb.h'" because the search path is (vcpkg installed root)/include/ while the header is in (vcpkg install root)/include/libusb-1.0/.

I seem to vaguely recall this is a long-standing bug with libusb in general, and that you need to pass in libusb-1.0 as an explicit include path sometimes.

Anyway, I'm getting tired of this problem

That is fine with me, tbh. I'm happy to revisit later if it comes up and do a release tonight.

Please make sure to merge #3 before releasing 1.0.0.

@tanriol
Copy link
Owner Author

tanriol commented Apr 15, 2020

Closed in favor of #5.

@tanriol tanriol closed this Apr 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants