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.

Sign up for GitHub

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

Jump to bottom

chore: replace atty with is-terminal #628

Merged
merged 1 commit into from
Apr 5, 2023
Merged

chore: replace atty with is-terminal #628

merged 1 commit into from
Apr 5, 2023

Conversation

jcgruenhage
Copy link
Contributor

@jcgruenhage jcgruenhage commented Nov 22, 2022 •
edited
Loading

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.

@roypat roypat mentioned this pull request Nov 22, 2022
Closed
@martinvonz martinvonz mentioned this pull request Nov 25, 2022
Merged
2 tasks
@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.

@iamjpotts iamjpotts mentioned this pull request Dec 29, 2022
Merged
vmx pushed a commit to multiformats/rust-multihash that referenced this pull request Jan 10, 2023
@iamjpotts
chore: add cargo deny to CI (#263)
88ad64e 
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
Closed
@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
@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.

@kalzoo kalzoo mentioned this pull request Apr 20, 2023
Merged
@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.

@john-h-kastner-aws john-h-kastner-aws mentioned this pull request May 23, 2023
Closed
3 tasks
@waywardmonkeys waywardmonkeys mentioned this pull request Aug 9, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lemmih lemmih lemmih approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jcgruenhage @lemmih @nbarrios1337 @bgianfo @nickelc @sunfishcode @dianpopa