Skip to content

Conversation

@edmorley
Copy link
Member

@edmorley edmorley commented Apr 29, 2025

Similar to #355 but for I/O errors from spawning a Command instead of from file operations.

Such Command I/O errors are only likely to happen in the case of a bug, so the error message now says as much. The error also now generates the program name from the command, so avoid it getting out of sync. The command's args are intentionally omitted, since they aren't relevant for errors that occur when spawning a command. (ie: For command not found, it's the program that's not found, not the args.)

GUS-W-12650236.

@edmorley edmorley added the enhancement New feature or request label Apr 29, 2025
@edmorley edmorley self-assigned this Apr 29, 2025
Similar to #355 but for I/O errors from spawning a `Command`
instead of from file operations.

Such Command I/O errors are only likely to happen in the case
of a bug, so the error message now says as much. The error
also now generates the program name from the command,
so avoid it getting out of sync. The command's args are
intentionally omitted, since they aren't relevant for errors
that occur when spawning a command. (ie: For command not
found, it's the program that's not found, not the args.)

GUS-W-12650236.
@edmorley edmorley force-pushed the improve-command-io-errors branch from 6f8b54a to dd4b1ae Compare April 29, 2025 14:06
@edmorley edmorley marked this pull request as ready for review April 29, 2025 14:57
@edmorley edmorley requested a review from a team as a code owner April 29, 2025 14:57
@edmorley edmorley enabled auto-merge (squash) April 29, 2025 17:40
Copy link

@schneems schneems left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Comments, no request for changes:

  • For debugging these failures, I wrote https://github.com/schneems/which_problem but haven't touched it in a long time. (An FYI if you're interested in poking at it or improving it).
  • Re the message in the commit: If you moved this into a different module and changed the visibility, you could guarantee that the string came from an &mut Command by forcing programmers to go through your interface:
pub(crate) struct CommandIoError {
    program: String,
    io_error: io::Error,
}

impl CommandIoError {
    pub(crate) fn new(command: &mut Command, io_error: io::Error) -> Self {
        Self {
            program: command.get_program().to_string_lossy().to_string(),
            io_error,
        }
    }

    pub(crate) fn program(&self) -> &str {
        &self.program
    }

    pub(crate) fn io_error(&self) -> &io::Error {
        &self.io_error
    }
}

Not needed, just mentioning an alternative.

@edmorley edmorley merged commit 7e6ae95 into main Apr 29, 2025
7 checks passed
@edmorley edmorley deleted the improve-command-io-errors branch April 29, 2025 19:38
@edmorley
Copy link
Member Author

edmorley commented Apr 29, 2025

For debugging these failures, I wrote https://github.com/schneems/which_problem but haven't touched it in a long time. (An FYI if you're interested in poking at it or improving it).

Yeah I'd glanced at it a while back - the reason I'm not using it here, is that these Command I/O errors are:
(a) internal errors that will mostly likely only ever be seen by buildpack maintainers if they've happened to introduce a bug (vs user-facing messaging)
(b) only encountered in fairly controlled circumstances (inside a CNB build's container, where there a lot more knowns than eg on a host machine if this project were instead say a CLI tool for end users)

...so we don't need the full feature-set supported by the which_problem crate - which makes it hard to justify the 500+ lines of non-test Rust in which_problem, plus a dozen transitive dependencies:

$ cargo tree
foo v0.1.0 (/Users/emorley/src/foo)
└── which_problem v0.1.0
    ├── is_executable v1.0.4
    ├── itertools v0.10.5
    │   └── either v1.15.0
    ├── ordered-float v3.9.2
    │   └── num-traits v0.2.19
    │       [build-dependencies]
    │       └── autocfg v1.4.0
    ├── rayon v1.10.0
    │   ├── either v1.15.0
    │   └── rayon-core v1.12.1
    │       ├── crossbeam-deque v0.8.6
    │       │   ├── crossbeam-epoch v0.9.18
    │       │   │   └── crossbeam-utils v0.8.21
    │       │   └── crossbeam-utils v0.8.21
    │       └── crossbeam-utils v0.8.21
    └── strsim v0.10.0

...vs the <20 lines of implementation here.

For projects that need the full feature-set of which_problem, this may be a worthwhile trade-off of course :-)

heroku-linguist bot added a commit that referenced this pull request May 2, 2025
## heroku/python

### Removed

- Removed support for the deprecated `runtime.txt` file. Python versions must now be specified using a `.python-version` file instead. ([#352](#352))
- Removed support for Ubuntu 20.04 (and thus Heroku-20 / `heroku/builder:20`). ([#358](#358))

### Changed

- Improved the error messages shown when `.python-version` contains an invalid Python version or stray invisible characters (such as ASCII control codes). ([#353](#353) and [#354](#354))
- Improved the error messages shown if I/O errors occur. ([#355](#355) and [#356](#356))
@heroku-linguist heroku-linguist bot mentioned this pull request May 2, 2025
heroku-linguist bot added a commit to heroku/cnb-builder-images that referenced this pull request May 2, 2025
## heroku/python

### Removed

- Removed support for the deprecated `runtime.txt` file. Python versions must now be specified using a `.python-version` file instead. ([#352](heroku/buildpacks-python#352))
- Removed support for Ubuntu 20.04 (and thus Heroku-20 / `heroku/builder:20`). ([#358](heroku/buildpacks-python#358))

### Changed

- Improved the error messages shown when `.python-version` contains an invalid Python version or stray invisible characters (such as ASCII control codes). ([#353](heroku/buildpacks-python#353) and [#354](heroku/buildpacks-python#354))
- Improved the error messages shown if I/O errors occur. ([#355](heroku/buildpacks-python#355) and [#356](heroku/buildpacks-python#356))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants