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

chore: replace atty with is-terminal #628

Merged
merged 1 commit into from Apr 5, 2023

Conversation

jcgruenhage
Copy link
Contributor

@jcgruenhage jcgruenhage commented Nov 22, 2022

atty is unmaintained and has a potential unaligned read. See https://github.com/rustsec/advisory-db/blob/main/crates/atty/RUSTSEC-2021-0145.md.

is-terminal is a replacement based on atty, with the soundness issue fixed and an (IMO) nicer to use API, mirroring what's available in the std lib on nightly with std::io::IsTerminal.

@lemmih
Copy link
Collaborator

lemmih commented Dec 6, 2022

@jcgruenhage The is-terminate crate somehow requires nightly when targeting wasi32. Do you know why?

@jcgruenhage
Copy link
Contributor Author

No clue, sorry. What's the error message you're getting? As far as I can tell, is-terminal should work one any Rust version >= 1.48.0 and not require nightly.

@lemmih
Copy link
Collaborator

lemmih commented Dec 6, 2022

No clue, sorry. What's the error message you're getting? As far as I can tell, is-terminal should work one any Rust version >= 1.48.0 and not require nightly.

It might use libc with a different set of features which then cause trouble in the errno crate. Will investigate.

vmx pushed a commit to multiformats/rust-multihash that referenced this pull request Jan 10, 2023
1. Locks the list of allowed licenses to the current set of dependency licenses
2. Surfaces two advisories as warnings (comments added to `deny.yml` for both)
3. Surfaces duplicate versions of the same dependency as warnings (comments added to `deny.yml` for both)

One of the advisories is for `atty` a dependency of `criterion` and tracked in an [issue in the criterion repo](bheisler/criterion.rs#628).
@briansmith briansmith mentioned this pull request Feb 15, 2023
@nbarrios1337
Copy link
Contributor

@jcgruenhage The is-terminate crate somehow requires nightly when targeting wasi32. Do you know why?

Did some digging on the wasi32 compilation error:

To summarize what this PR needs:

  • is-terminal would have to update its rustix dependency once rustix releases a version with the errno version bump change
  • This PR would then need to update to that is-terminal version in order for it to not fail on the aforementioned CI error

@bgianfo
Copy link

bgianfo commented Mar 13, 2023

To summarize what this PR needs:

  • is-terminal would have to update its rustix dependency once rustix releases a version with the errno version bump change
  • This PR would then need to update to that is-terminal version in order for it to not fail on the aforementioned CI error

Looks like is-terminal merged an update to it's rustix version, but hasn't quite cut a new release yet. sunfishcode/is-terminal#20

@nickelc
Copy link

nickelc commented Mar 29, 2023

A new version has just been released.
https://crates.io/crates/is-terminal/0.4.6

@jcgruenhage jcgruenhage force-pushed the replace-atty-with-is-terminal branch from 86e27bd to f240862 Compare March 29, 2023 07:43
@jcgruenhage
Copy link
Contributor Author

@nickelc thanks for the notification, I've pulled in that release and rebased on latest master. Hopefully that should be merge-able now?

@nbarrios1337
Copy link
Contributor

Looks like the csv crate updated their MSRV to rustc 1.60 as of csv v1.2.0. From my understanding, criterion would either need to (a) Bump its own MSRV to 1.60 or (b) set a stricter version requirement for its csv dependency (i.e. ~1.1 in the Cargo.toml)

@lemmih
Copy link
Collaborator

lemmih commented Mar 30, 2023

Looks like the csv crate updated their MSRV to rustc 1.60 as of csv v1.2.0. From my understanding, criterion would either need to (a) Bump its own MSRV to 1.60 or (b) set a stricter version requirement for its csv dependency (i.e. ~1.1 in the Cargo.toml)

Bumping the MSRV would be the easiest solution. Anyone up for that task?

@nbarrios1337
Copy link
Contributor

PR made to bump the MSRV: #665

@lemmih
Copy link
Collaborator

lemmih commented Apr 2, 2023

Hmm, it appears that rustix does not support wasm when using stable rust.

@sunfishcode
Copy link

Ah, that's a bug in rustix. I've submitted bytecodealliance/rustix#589 to fix it.

@sunfishcode
Copy link

The wasm32-wasi build on stable Rust is now fixed in rustix 0.37.7 and 0.36.12.

@lemmih
Copy link
Collaborator

lemmih commented Apr 4, 2023

The wasm32-wasi build on stable Rust is now fixed in rustix 0.37.7 and 0.36.12.

Nice!

@nbarrios1337
Copy link
Contributor

Perhaps a small commit for the CI to rerun is unpinning the tempfile dep? I believe it was pinned in #665 because of the same rustix bug.

@jcgruenhage jcgruenhage force-pushed the replace-atty-with-is-terminal branch from b7ce009 to 4f00f0b Compare April 5, 2023 08:23
@jcgruenhage
Copy link
Contributor Author

I just re-run the CI by rebasing and force-pushing instead. CI seems happy now!

@lemmih lemmih merged commit a211230 into bheisler:master Apr 5, 2023
5 checks passed
@dianpopa
Copy link

It s great to see this merged! Is it possible to also create an official tag for the change? (I expect 0.4.1)?
We use this in Firecracker for benchmarking our versionize functionality.
Thanks!

@lemmih
Copy link
Collaborator

lemmih commented Apr 11, 2023

New releases can only be made by @bheisler.

@dianpopa
Copy link

@bheisler can you please release a new version that includes this fix? Thanks!

@lemmih
Copy link
Collaborator

lemmih commented May 10, 2023

@bheisler can you please release a new version that includes this fix? Thanks!

I have permission to release a new version. Will need to review this PR first (#679) and then update the CHANGELOG. A new is coming but it'll come faster with help.

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

7 participants