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

Windows support. #2

Merged
merged 1 commit into from
Mar 4, 2015
Merged

Conversation

retep998
Copy link
Contributor

Fixes #1

@conradkleinespel
Copy link
Owner

@retep998 Thanks a lot for taking the time to contribute 👍 I'm reviewing your PR right now and will comment again when I'm done.

@@ -9,5 +9,10 @@ homepage = "https://github.com/conradkleinespel/rustastic-password"
readme = "README.md"
keywords = ["read", "password"]

[dependencies]
Copy link
Owner

Choose a reason for hiding this comment

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

The removal of these two lines break builds on UNIX. Is there a particular reason you removed these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To make it build for myself.
Anyway, I have re-added that dependency, but target specific.

Signed-off-by: Peter Atashian <[email protected]>
@retep998
Copy link
Contributor Author

Comments have been added and unsafe has been localized to where needed.

@conradkleinespel
Copy link
Owner

@retep998 Awesome 👍 Thanks for the hard work you put in. I don't have access to OSX or Windows right now. But I'll test this ASAP and comment again then.

termios = "0.0.2"
[target.aarch64-unknown-linux-gnu.dependencies]
termios = "0.0.2"
[target.arm-unknown-linux-gnueabihf.dependencies]
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a soft-float version of this as well: arm-unknown-linux-gnueabi.

@conradkleinespel
Copy link
Owner

@retep998 Just a quick update: I haven't had the time yet to review this on OSX/Windows. I will soon though.

@dcuddeback @retep998 I think the dependencies listed in Cargo.toml are getting a little messy indeed. Until a better solution is available in Cargo, I would suggest accepting this "messy" Cargo.toml so that it works until we have a better solution available. Later, if Cargo allows an easier way to do that or if @dcuddeback 's library compile on Windows and @retep998 's libraries compile on UNIX, we can just get rid of the "messy" declarations altogether and put some more #[cfg(...)] in the code.

@dcuddeback
Copy link
Contributor

@conradkleinespel The "messy" dependencies is known to the Cargo team. It's mentioned as one of the drawbacks in the RFC: "As can be seen in the example repository, platform dependencies are quite verbose and are difficult to work with when you actually want a negation instead of a positive platform to include." (https://github.com/rust-lang/rfcs/blob/master/text/0403-cargo-build-command.md#drawbacks) I think better syntax will come in the future, as they mentioned supporting wildcards in the future when approving the RFC: https://github.com/rust-lang/meeting-minutes/blob/master/weekly-meetings/2014-10-30.md#cargo-rfc.

I don't know much about Windows development. I don't think it supports the termios API. If it does, I'd welcome adding Windows support to the termios crate.

@retep998
Copy link
Contributor Author

@dcuddeback The Windows console API is completely different to the posix model. While someone could write a higher level API that can abstract across the two models, a lower level library such as your termios crate will never be able to work on Windows.

@dcuddeback
Copy link
Contributor

@retep998 That's what I assumed was the case. Thanks for confirming.

@conradkleinespel
Copy link
Owner

@retep998 Quick update: I haven't had any time for OSS last week. But I haven't forgotten about this PR and will test it soon.

conradkleinespel added a commit that referenced this pull request Mar 4, 2015
@retep998 Sorry this took so long. Looks good for me.
@conradkleinespel conradkleinespel merged commit a0307b7 into conradkleinespel:master Mar 4, 2015
This was referenced Jun 24, 2015
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.

Windows support
3 participants