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

Bump MSRV to 1.48.0 #146

Merged
merged 8 commits into from
Dec 7, 2022
Merged

Bump MSRV to 1.48.0 #146

merged 8 commits into from
Dec 7, 2022

Conversation

messense
Copy link
Contributor

@messense messense commented Dec 6, 2022

Per #134 (comment)

Closes #139

@messense
Copy link
Contributor Author

messense commented Dec 6, 2022

The macOS test failure should be easy to fix by use an old version of Xcode (like eminence/terminal-size@4cd679a), wasm32-wasi build failure is caused by rust-lang/rust#103306 which was stabilized in 1.65.0, we can switch to stable.

I'm not sure how to deal with the wasm32-unknown-unknown build failure.

@LingMan
Copy link
Contributor

LingMan commented Dec 6, 2022

wasm32-unknown-unknown support for errno was discussed in lambda-fairy/rust-errno#21.

@mitsuhiko
Copy link
Collaborator

Does terminal_size actually work on WASI? Because if all it does is have some hardcoded fallback then the sanest option would probably be to just stop using that crate for WASI.

@mitsuhiko
Copy link
Collaborator

mitsuhiko commented Dec 6, 2022

Now that I looked closer at this I'm not a huge fan of the fact that terminal_size 0.2 pulls in that many dependencies for what should be a rather simple task. I wonder if it wouldn't be better to switch to manual implementations again rather than using terminal_size. This was the original implementation before the change in d345166 anyways.

@messense
Copy link
Contributor Author

messense commented Dec 6, 2022

Does terminal_size actually work on WASI?

@eminence any idea?

@LingMan
Copy link
Contributor

LingMan commented Dec 6, 2022

It pulls in rustix instead of libc and windows-sys instead of winapi. Most of the indirect dependencies only get compiled on specific targets though (like errno-dragonfly which is only compiled on DragonflyBSD). #133 (comment) mentioned that console should switch to those too and I intended to give that a shot.

@LingMan
Copy link
Contributor

LingMan commented Dec 6, 2022

From a quick scan of the terminal_size repo it doesn't look like anyone ever even asked for wasm. Notably console doesn't try to call terminal_size on wasm either. It just reports None

console/src/wasm_term.rs

Lines 22 to 24 in 9c724ae

pub fn terminal_size(_out: &Term) -> Option<(u16, u16)> {
None
}

So, yeah, looks like the simplest solution is to not compile terminal_size on wasm targets.

@eminence
Copy link

eminence commented Dec 6, 2022

Does terminal_size actually work on WASI?

@eminence any idea?

No, terminal_size doesn't target WASI at all. I'm not sure if WASI today has any interfaces to get things like "terminal size". @sunfishcode -- Do you know if this is something that WASI might support in the future?

@messense I'm now thinking that terminal_size should just return None on WASI targets. Would that be useful for you here? (Edit: question is for @mitsuhiko too)

@sunfishcode
Copy link

@eminence That's right; WASI does not currently have APIs for returning the terminal size. It may in the future though.

@mitsuhiko
Copy link
Collaborator

I would say for now the terminal_size dependency should at least be removed for wasm. But quite frankly at this point I feel like I would like to get rid of terminal_size entirely. Getting the terminal sizes is easy enough without all those dependencies. Can we just vendor the code that we need?

@eminence
Copy link

eminence commented Dec 6, 2022

The code is really very small, so I think you should be able to very easily drop terminal_size and just include the relevant code directly in console.

@mitsuhiko mitsuhiko merged commit 7febd3c into console-rs:master Dec 7, 2022
@messense messense deleted the msrv-1.48 branch December 7, 2022 09:03
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.

5 participants