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

Issues with the test suite #18

Open
orhun opened this issue Jan 1, 2024 · 12 comments
Open

Issues with the test suite #18

orhun opened this issue Jan 1, 2024 · 12 comments

Comments

@orhun
Copy link
Contributor

orhun commented Jan 1, 2024

Hello! 🐻 I recently came across your blog post and discovered this project. I liked it and decided to package it for Linux distributions.

However, I came across a couple of problems while running the test suite:

  1. Tests require cargo-run-bin to be installed which means in an isolated environment (chroot), I am not able to run the tests without installing cargo-run-bin to the PATH. I would expect a parameter to tweak this, such as setting a custom path for the binary instead of straight up running cargo bin.

  2. I am getting a bunch of warnings about the aliases that are set in this repo:

warning: user-defined alias `nextest` is shadowing an external subcommand found at: `/usr/bin/cargo-nextest`
This was previously accepted but is being phased out; it will become a hard error in a future release.
For more information, see issue #10049 <https://github.com/rust-lang/cargo/issues/10049>.

warning: user-defined alias `binstall` is shadowing an external subcommand found at: `/usr/bin/cargo-binstall`
This was previously accepted but is being phased out; it will become a hard error in a future release.
For more information, see issue #10049 <https://github.com/rust-lang/cargo/issues/10049>.
  1. I realized the integration tests are being run like unit tests and the install path is hardcoded to target/debug. I made an attempt to fix this at refactor: run tests as integration test #17

Thanks for the nice tool!

@dustinblackman
Copy link
Owner

dustinblackman commented Jan 1, 2024

Hey man, big fan of your stuff!

Tests require cargo-run-bin to be installed which means in an isolated environment (chroot), I am not able to run the tests without installing cargo-run-bin to the PATH. I would expect a parameter to tweak this, such as setting a custom path for the binary instead of straight up running cargo bin.

That's a good callout. I see you got it fixed up in the PR you submitted, thanks!

I am getting a bunch of warnings about the aliases that are set in this repo:

This is an expected behaviour. The alias' in .cargo/config.toml were they best way I could find to get the same experience of installing cargo extension as if you were installing globally, but the drawback of this is you're expected to go all-in on using cargo-run-bin, meaning no longer having the extensions installed globally, otherwise you get those warnings.

@dustinblackman
Copy link
Owner

I liked it and decided to package it for Linux distributions.

Watcha got in mind? I have it on my TODO list to do something similar to this, but just haven't put the effort in yet.

@orhun
Copy link
Contributor Author

orhun commented Jan 1, 2024

I actually packaged it for Arch Linux already: https://archlinux.org/packages/extra/x86_64/cargo-run-bin/

Also submitted a merge request to Alpine: https://gitlab.alpinelinux.org/alpine/aports/-/merge_requests/58242

Will submit another PR here (for updating the docs) once it makes it into Alpine repos.

@dustinblackman
Copy link
Owner

Wow you're fast! That's awesome, thank you!

@orhun
Copy link
Contributor Author

orhun commented Jan 2, 2024

Looks like we have to wait until Rust 1.74 is available on Alpine :/

error: package `cargo-run-bin v1.7.1 (/builds/orhun/aports/testing/cargo-run-bin/src/cargo-run-bin-1.7.1)` cannot be built because it requires rustc 1.74.0 or newer, while the currently active rustc version is 1.72.1

@NobodyXu
Copy link

NobodyXu commented Jan 2, 2024

Maybe you can use cargo-binstall to install pre-built cargo-run-bin ?

There's also an official pre-built cargo-binstall and an official one-line script (it downloads and runs a very simple and easy to review script) for installing cargo-binstall

@orhun
Copy link
Contributor Author

orhun commented Jan 2, 2024

Good idea but unfortunately we cannot do that, the Alpine packages should be built from source.

@NobodyXu
Copy link

NobodyXu commented Jan 2, 2024

Good idea but unfortunately we cannot do that, the Alpine packages should be built from source.

Aha sorry, I know that for distro it has to be built from source, I am suggesting an alternative to installing from distro for now (until Alpine rustc is new enough).

IMO many rust crates are just easier to install and upgrade by using cargo-install or cargo-binstall, since they often require latest cargo and often updates frequently.

@NobodyXu
Copy link

NobodyXu commented Jan 2, 2024

For cargo-run-bin, maybe it makes sense to try lowering the msrv?

The msrv should be for cargo-run-bin by having a Cargo.lock to latest dependencies that is available on the msrv (there's a cargo ticket for automatically doing this).

Then users who need to build on msrv can simply build/install with --locked, and other can just install without it or use cargo-binstall or download pre-built binaries directly which uses latest version (via another Cargo.lock.latest?).

@orhun
Copy link
Contributor Author

orhun commented Jan 2, 2024

Yeah, lowering the MSRV is definitely an option. Currently 1.73.0 fails due to these errors for me:

│ error[E0277]: the trait bound `Stdio: From<Stderr>` is not satisfied                                                                       │
│   --> src/binary.rs:26:17                                                                                                                  │
│    |                                                                                                                                       │
│ 26 |         .stdout(io::stderr())                                                                                                         │
│    |          ------ ^^^^^^^^^^^^ the trait `From<Stderr>` is not implemented for `Stdio`                                                  │
│    |          |                                                                                                                            │
│    |          required by a bound introduced by this call                                                                                  │
│    |                                                                                                                                       │
│    = help: the following other types implement trait `From<T>`:                                                                            │
│              <Stdio as From<File>>                                                                                                         │
│              <Stdio as From<OwnedFd>>                                                                                                      │
│              <Stdio as From<ChildStdin>>                                                                                                   │
│              <Stdio as From<ChildStdout>>                                                                                                  │
│              <Stdio as From<ChildStderr>>                                                                                                  │
│    = note: required for `Stderr` to implement `Into<Stdio>`                                                                                │
│ note: required by a bound in `std::process::Command::stdout`                                                                               │
│   --> /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/std/src/process.rs:898:5                                                     │
│                                                                                                                                            │
│ error[E0277]: the trait bound `Stdio: From<Stderr>` is not satisfied                                                                       │
│   --> src/binary.rs:78:17                                                                                                                  │
│    |                                                                                                                                       │
│ 78 |         .stdout(io::stderr())                                                                                                         │
│    |          ------ ^^^^^^^^^^^^ the trait `From<Stderr>` is not implemented for `Stdio`                                                  │
│    |          |                                                                                                                            │
│    |          required by a bound introduced by this call                                                                                  │
│    |                                                                                                                                       │
│    = help: the following other types implement trait `From<T>`:                                                                            │
│              <Stdio as From<File>>                                                                                                         │
│              <Stdio as From<OwnedFd>>                                                                                                      │
│              <Stdio as From<ChildStdin>>                                                                                                   │
│              <Stdio as From<ChildStdout>>                                                                                                  │
│              <Stdio as From<ChildStderr>>                                                                                                  │
│    = note: required for `Stderr` to implement `Into<Stdio>`                                                                                │
│ note: required by a bound in `std::process::Command::stdout`                                                                               │
│   --> /rustc/cc66ad468955717ab92600c770da8c1601a4ff33/library/std/src/process.rs:898:5                                                     │
│                                                                                                                                            │
│ For more information about this error, try `rustc --explain E0277`.                                                                        │
│ error: could not compile `cargo-run-bin` (lib) due to 2 previous errors 

@NobodyXu
Copy link

NobodyXu commented Jan 2, 2024

Hmmm IMO it can be workaround by doing this:

Add this to Cargo.toml:

[dependencies]
rustversion = "1.0"

Then:

use std::{io, process};

#[rustversion::since(1.74)]
fn stderr_to_stdio() -> io::Result<process::Stdio> {
    Ok(io::stderr().into())
}

#[rustversion::before(1.74)]
#[cfg(unix)]
fn stderr_to_stdio() -> io::Result<process::Stdio> {
    use std::os::fd::AsFd;

    Ok(io::stderr().as_fd().try_clone_to_owned()?.into())
}

#[rustversion::before(1.74)]
#[cfg(windows)]
fn stderr_to_stdio() -> io::Result<process::Stdio> {
    use std::os::windows::io::AsHandle;

    Ok(io::stderr().as_handle().try_clone_to_owned()?.into())
}

    // then:
    command.stdout(stderr_to_stdio()?);

It's easier than I thought, with BorrowedFd<'_> and BorrowedHandle<'_> stablised in rust 1.63, it's no longer necessary to use mem::ManuallyDrop::new(unsafe { File::from_raw_fd(io::stdio().as_raw_fd()) }).try_clone()?.into() just to clone the underlying fd and cast it to Stdio.

@dustinblackman
Copy link
Owner

@NobodyXu Nice, that's super clean, thanks!

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

No branches or pull requests

3 participants