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 ConPty APIs #699

Merged
merged 4 commits into from Dec 28, 2018
Merged

Add ConPty APIs #699

merged 4 commits into from Dec 28, 2018

Conversation

davidhewitt
Copy link
Contributor

Closes #695.

I added the bare minimum to cover those three functions. These APIs will only work on the very newest Windows. Does anything need to be done to feature gate them?

@retep998
Copy link
Owner

There's no need to feature gate things based on Windows version because extern functions are only linked to when used. That said, if you do use these functions then your program won't run at all on older Windows versions unless you use GetProcAddress to obtain pointers to the functions instead of using these extern functions directly.

@davidhewitt
Copy link
Contributor Author

davidhewitt commented Oct 21, 2018

Cool. I'd rather not use GetProcAddress but it's going to be quite a small segment of windows users able to use these currently! 😄

Do you need anything else to make this PR mergeable?

@chyyran
Copy link

chyyran commented Nov 19, 2018

1809 is now GA, so ConPTY should be available on updated Windows 10 systems.

src/um/wincon.rs Outdated Show resolved Hide resolved
@davidhewitt
Copy link
Contributor Author

I've moved HPCON to wincontypes.rs and introduced a new feature as discussed. Turns out a number of the struct definitions in wincon.rs are also defined in wincontypes.h, so I've moved those at the same time.

@retep998
Copy link
Owner

retep998 commented Nov 24, 2018

When moving existing definitions to a different header, please put in pub use statements so that code still relying on the definitions in the old location is not broken.

Also when adding a new header, you also have to update the feature array in build.rs.

@davidhewitt
Copy link
Contributor Author

Thanks - will fixup tomorrow morning :)

@davidhewitt
Copy link
Contributor Author

davidhewitt commented Nov 25, 2018

Have corrected all that now. Many thanks for your patience in helping refine all the bits I originally missed. Would a separate PR be useful to extend the notes in CONTRIBUTING to make those points clearer?

@davidhewitt
Copy link
Contributor Author

Just wondering if there's anything else you'd like to see to have this merged?

@davidhewitt
Copy link
Contributor Author

@retep998 just pinging to say we're hoping to merge alacritty/alacritty#1762 but would like to see these definitions merged into winapi first (we're using this patch I'm providing). Please let me know if there's anything left I can do to help this PR merge.

@retep998 retep998 merged commit c8a6b68 into retep998:0.3 Dec 28, 2018
@davidhewitt
Copy link
Contributor Author

Thanks!

@davidhewitt davidhewitt deleted the conpty branch December 29, 2018 09:26
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.

None yet

3 participants