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

lib/types: Fix path type check #77133

Merged
merged 1 commit into from Jan 7, 2020
Merged

lib/types: Fix path type check #77133

merged 1 commit into from Jan 7, 2020

Conversation

@Infinisil
Copy link
Member

@Infinisil Infinisil commented Jan 6, 2020

Motivation for this change

Previously when this function was called without a value coercible to a
string it would throw an error instead of returning false. Now it does.

As a result this now allows the use of a type like either path attrs
without it erroring out when a definition is an attribute set.

The warning about there not being a isPath primop was removed because
this is not the case anymore, there is builtins.isPath. But also there
always was builtins.typeOf x == "path" that could've been used
instead. However the path type now stands for more than just path types,
but absolute paths in general.

Fixes #76977, relevant for #76861

Ping @roberth

Things done
  • Tested that it doesn't throw an error anymore with attribute sets
@Infinisil Infinisil requested review from edolstra and nbp as code owners Jan 6, 2020
Previously when this function was called without a value coercible to a
string it would throw an error instead of returning false. Now it does.

As a result this now allows the use of a type like `either path attrs`
without it erroring out when a definition is an attribute set.

The warning about there not being a isPath primop was removed because
this is not the case anymore, there is builtins.isPath. But also there
always was `builtins.typeOf x == "path"` that could've been used
instead. However the path type now stands for more than just path types,
but absolute paths in general.
@Infinisil Infinisil force-pushed the Infinisil:fix-path-check branch from 3347fef to d7a109b Jan 6, 2020
@roberth
roberth approved these changes Jan 7, 2020
@Infinisil Infinisil merged commit 65872f4 into NixOS:master Jan 7, 2020
12 checks passed
12 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@Infinisil Infinisil deleted the Infinisil:fix-path-check branch Jan 7, 2020
@roberth roberth mentioned this pull request Jan 7, 2020
2 of 2 tasks complete
@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Jan 8, 2020

Backported in 86bbfc0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.