Skip to content

chore: replace atty with is-terminal#628

Merged
lemmih merged 1 commit intobheisler:masterfrom
jcgruenhage:replace-atty-with-is-terminal
Apr 5, 2023
Merged

chore: replace atty with is-terminal#628
lemmih merged 1 commit intobheisler:masterfrom
jcgruenhage:replace-atty-with-is-terminal

Conversation

@jcgruenhage
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

lemmih commented Dec 6, 2022

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

@jcgruenhage
Copy link
Copy Markdown
Contributor Author

jcgruenhage 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.

@lemmih
Copy link
Copy Markdown
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
Copy Markdown
Contributor

nbarrios1337 commented Feb 18, 2023

@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
Copy Markdown

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
Copy Markdown

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
Copy Markdown
Contributor Author

jcgruenhage commented Mar 29, 2023

@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
Copy Markdown
Contributor

nbarrios1337 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)

@lemmih
Copy link
Copy Markdown
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
Copy Markdown
Contributor

nbarrios1337 commented Mar 31, 2023

PR made to bump the MSRV: #665

@lemmih
Copy link
Copy Markdown
Collaborator

lemmih commented Apr 2, 2023

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

@sunfishcode
Copy link
Copy Markdown

sunfishcode commented Apr 2, 2023

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

@sunfishcode
Copy link
Copy Markdown

sunfishcode commented Apr 4, 2023

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

@lemmih
Copy link
Copy Markdown
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
Copy Markdown
Contributor

nbarrios1337 commented Apr 4, 2023

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
Copy Markdown
Contributor Author

jcgruenhage commented Apr 5, 2023

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
Copy Markdown

dianpopa commented Apr 10, 2023

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
Copy Markdown
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
Copy Markdown

dianpopa commented Apr 21, 2023

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

@lemmih
Copy link
Copy Markdown
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