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

Ensure cache directory is an absolute path #468

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bengsparks
Copy link

@bengsparks bengsparks commented Feb 13, 2025

Closes #467. Forces an absolute path by calling Path.absolute.
Tested by repeating the MREs from the aforementioned issue.

The early return of return TemporaryDirectory() a few lines above the diff in the first commit does not affect this,
as TemporaryDirectory will always return an absolute pathname because it uses the same rules as mkdtemp.

Specifically, for Python 3.11 and lower, mkdtemp will always return an absolute path when dir is None, and since Python 3.12, will always return an absolute pathname.

NOTE: It appears that when using env HOME=., a later call to nom-shell after the packages have been built suffers from a similar issue.
error: string './.config/nixpkgs/config.nix' doesn't represent an absolute path.

The call in question
$ /etc/profiles/per-user/ben/bin/nom-shell \
    --argstr local-system aarch64-darwin \
    --argstr nixpkgs-path /Users/ben/coding/nixpkgs/nixpkgs/.cache/nixpkgs-review/pr-366438-9/nixpkgs \
    --argstr nixpkgs-config-path /var/folders/mj/_03p5tkj30g1y7518fx_jydm0000gn/T/tmpfh76byaf.nix \
    --argstr attrs-path /Users/ben/coding/nixpkgs/nixpkgs/.cache/nixpkgs-review/pr-366438-9/attrs.nix \
    --nix-path 'nixpkgs=/Users/ben/coding/nixpkgs/nixpkgs/.cache/nixpkgs-review/pr-366438-9/nixpkgs nixpkgs-overlays=/var/folders/mj/_03p5tkj30g1y7518fx_jydm0000gn/T/tmp3844tuxd' \
    /Users/ben/coding/prs/nixpkgs-review/nixpkgs_review/nix/review-shell.nix

I suspect that this issue lies outside of this codebase, as searches for config.nix don't yield any matches, and the nom-shell CLI call does not reference such a path.
I had initially suspected this line, but applying .absolute() does not yield any change in behaviour.

A suspect would be this subprocess.run wrapper, as when env is None, the new subprocess will inherit the current process’ environment.

@bengsparks bengsparks changed the title nixpkgs-review: ensure cache directory is an absolute path Ensure cache directory is an absolute path Feb 13, 2025
@SuperSandro2000
Copy link
Collaborator

FYI: you don't need to open an issue just for the process

@SuperSandro2000
Copy link
Collaborator

Specifically, for Python 3.11 and lower, mkdtemp will always return an absolute path when dir is None, and since Python 3.12, will always return an absolute pathname.

Do we even want to support older versions? But maybe for the time being a good idea anyway.

@bengsparks
Copy link
Author

Versions of Python older than 3.12 are actually not supported; this file uses the type keyword to define a type alias, something that was first introduced in Python 3.12.

Anything older will fail with type System = str, SyntaxError: invalid syntax

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

Successfully merging this pull request may close these issues.

nixpkgs-review fails when the cache directory is set to a relative path
2 participants