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

Implement "static" feature flag for FTD2XX static linking. #15

Merged
merged 1 commit into from
Mar 25, 2021

Conversation

Grinkers
Copy link
Collaborator

@Grinkers Grinkers commented Feb 8, 2021

This sets the default configuration to dynamically link with FTD2XX on all platforms.

#10

@newAM
Copy link
Member

newAM commented Feb 9, 2021

I tried to integrate this changeset into an existing project that expects the previous linking format (static on linux, dynamic on windows), I put this in my Cargo.toml (repo):

[target.'cfg(target_os = "windows")'.dependencies.libftd2xx-ffi]
git = "https://github.com/Grinkers/libftd2xx-ffi-rs.git"
branch = "main"
features = []

[target.'cfg(target_os = "linux")'.dependencies.libftd2xx-ffi]
git = "https://github.com/Grinkers/libftd2xx-ffi-rs.git"
branch = "main"
features = ["static"]

and it seems the static feature is getting passed to both Linux and Windows targets.

This appears to be a known cargo issue, and there will be a feature resolver version 2 in 1.51 that fixes this.

it avoids unifying in these situations: Features enabled on platform-specific dependencies for targets not currently being built are ignored.

Would you be ok with waiting until 1.51 to merge this (~7 weeks)? I want to make sure there is a path forward for existing users to retain existing linking functionality.

Edit: Alternatively, maybe a separate branch and beta release until 1.51 is available? What do you think?

@Grinkers
Copy link
Collaborator Author

Grinkers commented Feb 9, 2021

Is it planned for 1.51? I certainly see no rush for this to be merged upstream. I did it for selfish reasons and don't mind just pointing to my fork...

If it's only a couple more releases on stable, then let's just wait. Seems like they're pushing for resolver 2 to be default in edition 2021, so more likely it'll get in sooner than later. Bonus points if we can just set the edition to 2021 too!

@newAM
Copy link
Member

newAM commented Feb 9, 2021

Is it planned for 1.51? I certainly see no rush for this to be merged upstream.

It is present in 1.51 nightly.
Looks like this is still changing by the hour, but it should make it into the 1.51 release notes being drafted right now: rust-lang/rust#81917 (comment)

If it's only a couple more releases on stable, then let's just wait.

Sounds good!

@newAM newAM added enhancement New feature or request blocked 1.51 Blocked until rust 1.51 is stable labels Feb 9, 2021
@Grinkers Grinkers force-pushed the main branch 2 times, most recently from 45e99b6 to b24aaa9 Compare March 16, 2021 07:04
This sets the default configuration to dynamically link with FTD2XX on all platforms.
@newAM
Copy link
Member

newAM commented Mar 25, 2021

1.51 is here 🎊

There is a path forward now to retaining different linking strategies on different platforms with resolver = "2", works in the test repository I linked above.

Anything you want to add before this gets merged? Also feel free to add yourself as an author of the crate if you want!

@Grinkers
Copy link
Collaborator Author

I'm glad Resolver 2 got into 1.51!

I have nothing else to add here. I'll probably poke at #1 again once this gets merged in. It'll make it a lot easier to make the version example to compile, run, and test. I'm fine just being a contributor.

@newAM newAM merged commit 152b1eb into ftdi-rs:main Mar 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked 1.51 Blocked until rust 1.51 is stable enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants