Skip to content

Improve the error messages for file I/O errors #355

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

Merged
merged 1 commit into from
Apr 29, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Changed

- Improved the error messages shown when `.python-version` contains an invalid Python version or stray invisible characters (such as ASCII control codes). ([#353](https://github.com/heroku/buildpacks-python/pull/353) and [#354](https://github.com/heroku/buildpacks-python/pull/354))
- Improved the error messages shown if I/O errors occur. ([#355](https://github.com/heroku/buildpacks-python/pull/355))

## [0.26.1] - 2025-04-08

Expand Down
11 changes: 6 additions & 5 deletions src/detect.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::io;
use crate::utils::{self, FileExistsError};
use std::path::Path;

/// Filenames that if found in a project mean it should be treated as a Python project,
Expand Down Expand Up @@ -40,11 +40,10 @@ const KNOWN_PYTHON_PROJECT_FILES: [&str; 23] = [

/// Returns whether the specified project directory is that of a Python project, and so
/// should pass buildpack detection.
pub(crate) fn is_python_project_directory(app_dir: &Path) -> io::Result<bool> {
pub(crate) fn is_python_project_directory(app_dir: &Path) -> Result<bool, FileExistsError> {
// Until `Iterator::try_find` is stabilised, this is cleaner as a for loop.
for filename in KNOWN_PYTHON_PROJECT_FILES {
let path = app_dir.join(filename);
if path.try_exists()? {
if utils::file_exists(&app_dir.join(filename))? {
return Ok(true);
}
}
Expand All @@ -71,7 +70,9 @@ mod tests {

#[test]
fn is_python_project_directory_io_error() {
assert!(is_python_project_directory(Path::new("tests/fixtures/empty/.gitkeep")).is_err());
// We pass a path containing a NUL byte as an easy way to trigger an I/O error.
let err = is_python_project_directory(Path::new("\0/invalid")).unwrap_err();
assert_eq!(err.path, Path::new("\0/invalid/.python-version"));
}

#[test]
Expand Down
18 changes: 11 additions & 7 deletions src/django.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,17 @@
use crate::FileExistsError;
use crate::utils::{self, CapturedCommandError, StreamedCommandError};
use indoc::indoc;
use libcnb::Env;
use libherokubuildpack::log::log_info;
use std::io;
use std::path::Path;
use std::process::Command;

const MANAGEMENT_SCRIPT_NAME: &str = "manage.py";

pub(crate) fn is_django_installed(dependencies_layer_dir: &Path) -> io::Result<bool> {
dependencies_layer_dir.join("bin/django-admin").try_exists()
pub(crate) fn is_django_installed(dependencies_layer_dir: &Path) -> Result<bool, FileExistsError> {
// The Django package includes a `django-admin` entrypoint script, which we can use
// as a simple/fast way to check if Django is installed.
utils::file_exists(&dependencies_layer_dir.join("bin/django-admin"))
}

pub(crate) fn run_django_collectstatic(
Expand Down Expand Up @@ -54,8 +56,8 @@ pub(crate) fn run_django_collectstatic(
.map_err(DjangoCollectstaticError::CollectstaticCommand)
}

fn has_management_script(app_dir: &Path) -> io::Result<bool> {
app_dir.join(MANAGEMENT_SCRIPT_NAME).try_exists()
fn has_management_script(app_dir: &Path) -> Result<bool, FileExistsError> {
utils::file_exists(&app_dir.join(MANAGEMENT_SCRIPT_NAME))
}

fn has_collectstatic_command(app_dir: &Path, env: &Env) -> Result<bool, CapturedCommandError> {
Expand Down Expand Up @@ -87,7 +89,7 @@ fn has_collectstatic_command(app_dir: &Path, env: &Env) -> Result<bool, Captured
#[derive(Debug)]
pub(crate) enum DjangoCollectstaticError {
CheckCollectstaticCommandExists(CapturedCommandError),
CheckManagementScriptExists(io::Error),
CheckManagementScriptExists(FileExistsError),
CollectstaticCommand(StreamedCommandError),
}

Expand All @@ -110,6 +112,8 @@ mod tests {

#[test]
fn has_management_script_io_error() {
assert!(has_management_script(Path::new("tests/fixtures/empty/.gitkeep")).is_err());
// We pass a path containing a NUL byte as an easy way to trigger an I/O error.
let err = has_management_script(Path::new("\0/invalid")).unwrap_err();
assert_eq!(err.path, Path::new("\0/invalid/manage.py"));
}
}
133 changes: 84 additions & 49 deletions src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,10 @@ use crate::python_version::{
ResolvePythonVersionError,
};
use crate::python_version_file::ParsePythonVersionFileError;
use crate::utils::{CapturedCommandError, DownloadUnpackArchiveError, StreamedCommandError};
use crate::utils::{
CapturedCommandError, DownloadUnpackArchiveError, FileExistsError, FindBundledPipError,
ReadOptionalFileError, StreamedCommandError,
};
use indoc::{formatdoc, indoc};
use libherokubuildpack::log::log_error;
use std::io;
Expand Down Expand Up @@ -49,11 +52,12 @@ pub(crate) fn on_error(error: libcnb::Error<BuildpackError>) {

fn on_buildpack_error(error: BuildpackError) {
match error {
BuildpackError::BuildpackDetection(error) => on_buildpack_detection_error(&error),
BuildpackError::BuildpackDetection(error) | BuildpackError::DjangoDetection(error) => {
log_file_exists_error(error);
}
BuildpackError::Checks(error) => on_buildpack_checks_error(error),
BuildpackError::DeterminePackageManager(error) => on_determine_package_manager_error(error),
BuildpackError::DjangoCollectstatic(error) => on_django_collectstatic_error(error),
BuildpackError::DjangoDetection(error) => on_django_detection_error(&error),
BuildpackError::PipDependenciesLayer(error) => on_pip_dependencies_layer_error(error),
BuildpackError::PipLayer(error) => on_pip_layer_error(error),
BuildpackError::PoetryDependenciesLayer(error) => on_poetry_dependencies_layer_error(error),
Expand All @@ -64,14 +68,6 @@ fn on_buildpack_error(error: BuildpackError) {
}
}

fn on_buildpack_detection_error(error: &io::Error) {
log_io_error(
"Unable to complete buildpack detection",
"determining if the Python buildpack should be run for this application",
error,
);
}

fn on_buildpack_checks_error(error: ChecksError) {
match error {
ChecksError::ForbiddenEnvVar(name) => log_error(
Expand All @@ -89,11 +85,7 @@ fn on_buildpack_checks_error(error: ChecksError) {

fn on_determine_package_manager_error(error: DeterminePackageManagerError) {
match error {
DeterminePackageManagerError::CheckFileExists(io_error) => log_io_error(
"Unable to determine the package manager",
"determining which Python package manager to use for this project",
&io_error,
),
DeterminePackageManagerError::CheckFileExists(error) => log_file_exists_error(error),
DeterminePackageManagerError::MultipleFound(package_managers) => {
let files_found = package_managers
.into_iter()
Expand Down Expand Up @@ -141,16 +133,8 @@ fn on_determine_package_manager_error(error: DeterminePackageManagerError) {

fn on_requested_python_version_error(error: RequestedPythonVersionError) {
match error {
RequestedPythonVersionError::CheckRuntimeTxtExists(io_error) => log_io_error(
"Unable to check for runtime.txt",
"checking if a runtime.txt file exists",
&io_error,
),
RequestedPythonVersionError::ReadPythonVersionFile(io_error) => log_io_error(
"Unable to read .python-version",
"reading the .python-version file",
&io_error,
),
RequestedPythonVersionError::CheckRuntimeTxtExists(error) => log_file_exists_error(error),
RequestedPythonVersionError::ReadPythonVersionFile(error) => log_read_file_error(error),
RequestedPythonVersionError::ParsePythonVersionFile(error) => match error {
ParsePythonVersionFileError::InvalidVersion(version) => log_error(
"Invalid Python version in .python-version",
Expand Down Expand Up @@ -384,11 +368,7 @@ fn on_pip_layer_error(error: PipLayerError) {
"},
),
},
PipLayerError::LocateBundledPip(io_error) => log_io_error(
"Unable to locate the bundled copy of pip",
"locating the pip wheel file bundled inside the Python 'ensurepip' module",
&io_error,
),
PipLayerError::LocateBundledPip(error) => log_find_bundled_pip_error(error),
}
}

Expand Down Expand Up @@ -455,11 +435,7 @@ fn on_poetry_layer_error(error: PoetryLayerError) {
"},
),
},
PoetryLayerError::LocateBundledPip(io_error) => log_io_error(
"Unable to locate the bundled copy of pip",
"locating the pip wheel file bundled inside the Python 'ensurepip' module",
&io_error,
),
PoetryLayerError::LocateBundledPip(error) => log_find_bundled_pip_error(error),
}
}

Expand Down Expand Up @@ -501,14 +477,6 @@ fn on_poetry_dependencies_layer_error(error: PoetryDependenciesLayerError) {
}
}

fn on_django_detection_error(error: &io::Error) {
log_io_error(
"Unable to determine if this is a Django-based app",
"checking if the 'django-admin' command exists",
error,
);
}

fn on_django_collectstatic_error(error: DjangoCollectstaticError) {
match error {
DjangoCollectstaticError::CheckCollectstaticCommandExists(error) => match error {
Expand Down Expand Up @@ -537,11 +505,9 @@ fn on_django_collectstatic_error(error: DjangoCollectstaticError) {
},
),
},
DjangoCollectstaticError::CheckManagementScriptExists(io_error) => log_io_error(
"Unable to inspect Django configuration",
"checking if the 'manage.py' script exists",
&io_error,
),
DjangoCollectstaticError::CheckManagementScriptExists(error) => {
log_file_exists_error(error);
}
DjangoCollectstaticError::CollectstaticCommand(error) => match error {
StreamedCommandError::Io(io_error) => log_io_error(
"Unable to generate Django static files",
Expand Down Expand Up @@ -571,6 +537,8 @@ fn on_django_collectstatic_error(error: DjangoCollectstaticError) {
}
}

// This is now only used for Command I/O errors.
// TODO: Replace this with specialised handling for Command I/O errors.
fn log_io_error(header: &str, occurred_whilst: &str, io_error: &io::Error) {
// We don't suggest opening a support ticket, since a subset of I/O errors can be caused
// by issues in the application. In the future, perhaps we should try and split these out?
Expand All @@ -583,3 +551,70 @@ fn log_io_error(header: &str, occurred_whilst: &str, io_error: &io::Error) {
"},
);
}

fn log_file_exists_error(FileExistsError { path, io_error }: FileExistsError) {
let filepath = path.to_string_lossy();
let filename = path
.file_name()
.unwrap_or(path.as_os_str())
.to_string_lossy();

log_error(
format!("Unable to check if {filename} exists"),
formatdoc! {"
An I/O error occurred while checking if this file exists:
{filepath}

Details: {io_error}

Try building again to see if the error resolves itself.
"},
);
}

fn log_read_file_error(ReadOptionalFileError { path, io_error }: ReadOptionalFileError) {
let filepath = path.to_string_lossy();
let filename = path
.file_name()
.unwrap_or(path.as_os_str())
.to_string_lossy();

log_error(
format!("Unable to read {filename}"),
formatdoc! {"
An I/O error occurred while reading the file:
{filepath}

Details: {io_error}

Check the file's permissions and that it contains valid UTF-8.

Then try building again.
"},
);
}

fn log_find_bundled_pip_error(
FindBundledPipError {
bundled_wheels_dir,
io_error,
}: FindBundledPipError,
) {
let bundled_wheels_dir = bundled_wheels_dir.to_string_lossy();

// TODO: Decide whether we want users to file a bug or open a support ticket.
log_error(
"Unable to locate the Python stdlib's bundled pip",
formatdoc! {"
Couldn't find the pip wheel file bundled inside the Python
stdlib's `ensurepip` module, at:
{bundled_wheels_dir}

Details: {io_error}

This is an internal error. Please report it as a bug, here:
https://github.com/heroku/buildpacks-python/issues
"
},
);
}
5 changes: 2 additions & 3 deletions src/layers/pip.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::packaging_tool_versions::PIP_VERSION;
use crate::python_version::PythonVersion;
use crate::utils::StreamedCommandError;
use crate::utils::{FindBundledPipError, StreamedCommandError};
use crate::{BuildpackError, PythonBuildpack, utils};
use libcnb::Env;
use libcnb::build::BuildContext;
Expand All @@ -11,7 +11,6 @@ use libcnb::layer::{
use libcnb::layer_env::{LayerEnv, ModificationBehavior, Scope};
use libherokubuildpack::log::log_info;
use serde::{Deserialize, Serialize};
use std::io;
use std::path::Path;
use std::process::Command;

Expand Down Expand Up @@ -133,7 +132,7 @@ struct PipLayerMetadata {
#[derive(Debug)]
pub(crate) enum PipLayerError {
InstallPipCommand(StreamedCommandError),
LocateBundledPip(io::Error),
LocateBundledPip(FindBundledPipError),
}

impl From<PipLayerError> for libcnb::Error<BuildpackError> {
Expand Down
5 changes: 2 additions & 3 deletions src/layers/poetry.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::packaging_tool_versions::POETRY_VERSION;
use crate::python_version::PythonVersion;
use crate::utils::StreamedCommandError;
use crate::utils::{FindBundledPipError, StreamedCommandError};
use crate::{BuildpackError, PythonBuildpack, utils};
use libcnb::Env;
use libcnb::build::BuildContext;
Expand All @@ -11,7 +11,6 @@ use libcnb::layer::{
use libcnb::layer_env::{LayerEnv, ModificationBehavior, Scope};
use libherokubuildpack::log::log_info;
use serde::{Deserialize, Serialize};
use std::io;
use std::path::Path;
use std::process::Command;

Expand Down Expand Up @@ -134,7 +133,7 @@ struct PoetryLayerMetadata {
#[derive(Debug)]
pub(crate) enum PoetryLayerError {
InstallPoetryCommand(StreamedCommandError),
LocateBundledPip(io::Error),
LocateBundledPip(FindBundledPipError),
}

impl From<PoetryLayerError> for libcnb::Error<BuildpackError> {
Expand Down
6 changes: 3 additions & 3 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,13 @@ use crate::python_version::{
PythonVersionOrigin, RequestedPythonVersion, RequestedPythonVersionError,
ResolvePythonVersionError,
};
use crate::utils::FileExistsError;
use indoc::formatdoc;
use libcnb::build::{BuildContext, BuildResult, BuildResultBuilder};
use libcnb::detect::{DetectContext, DetectResult, DetectResultBuilder};
use libcnb::generic::{GenericMetadata, GenericPlatform};
use libcnb::{Buildpack, Env, buildpack_main};
use libherokubuildpack::log::{log_header, log_info, log_warning};
use std::io;

struct PythonBuildpack;

Expand Down Expand Up @@ -157,15 +157,15 @@ impl Buildpack for PythonBuildpack {
#[derive(Debug)]
pub(crate) enum BuildpackError {
/// I/O errors when performing buildpack detection.
BuildpackDetection(io::Error),
BuildpackDetection(FileExistsError),
/// Errors due to one of the environment checks failing.
Checks(ChecksError),
/// Errors determining which Python package manager to use for a project.
DeterminePackageManager(DeterminePackageManagerError),
/// Errors running the Django collectstatic command.
DjangoCollectstatic(DjangoCollectstaticError),
/// I/O errors when detecting whether Django is installed.
DjangoDetection(io::Error),
DjangoDetection(FileExistsError),
/// Errors installing the project's dependencies into a layer using pip.
PipDependenciesLayer(PipDependenciesLayerError),
/// Errors installing pip into a layer.
Expand Down
Loading