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: Allow paths as submodule values #76861

Merged
merged 3 commits into from Jan 12, 2020

Conversation

@Infinisil
Copy link
Member

@Infinisil Infinisil commented Jan 3, 2020

Motivation for this change

Because allowing paths as submodule values isn't always backwards compatible, that change from #75031 had to be reverted.

This restores that behavior but also fixes the backwards incompatibility in nixpkgs and adds a release note about it.

Ping @roberth @danbst @grahamc

Things done
  • Updated docs with this change
  • Ran lib/tests/modules.sh successfully (again, the revert actually broke a test for this very thing in there)
@Infinisil Infinisil requested review from edolstra and nbp as code owners Jan 3, 2020
Infinisil added a commit that referenced this pull request Jan 3, 2020
Until #76861 or so is merged
@Infinisil Infinisil force-pushed the Infinisil:paths-as-submodules branch from 1b09f89 to 5b6cdb8 Jan 3, 2020
@roberth
Copy link
Contributor

@roberth roberth commented Jan 3, 2020

Can't we solve the problem by flipping the either arguments where necessary?
In the long run parameters like this add up to a significant overhead for users and maintainers, in particular

  • documentation becomes harder to use
  • complexity of the module system logic
  • code breaking when eventually switching from submodule to submoduleWith

We should lean towards simplification and document any required changes to user modules in the release notes.

@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Jan 3, 2020

@roberth Tried that already, but unfortunately not as easy as that. Main problem is that types.path.check fails for things that can't be coerced to strings, and I don't see an easy way to fix this (we'd need some builtins.isCoercibleToString function). This is why it's in the second place in that either.

If you have a decent implementation of types.path.check then we could maybe use that instead and hope to fix all such either instances instead. But we then still can't fix either occurrences outside of nixpkgs.

@roberth
Copy link
Contributor

@roberth roberth commented Jan 5, 2020

Blocked on #76977 it seems. The builtin doesn't solve the other problem with path.check, which is adding intermediate cleanSourceWith sources to the store.

But we then still can't fix either occurrences outside of nixpkgs.

Can be done in the release notes. I don't expect it to be a big problem (famous last words?).
Alternatively we can document the temporary nature of the new parameter and deprecate submodule.

@Infinisil Infinisil mentioned this pull request Jan 6, 2020
1 of 1 task complete
@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Jan 7, 2020

Now that we could switch all either submodule path to either path submodule, a problem is that either submodule path won't work with future versions of nixpkgs, and either path submodule won't work with past versions of nixpkgs. This means projects defining such a type can't accommodate all different nixpkgs versions if they use such a type.

@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Jan 7, 2020

Uses are probably few though, so it might not matter much

@roberth
Copy link
Contributor

@roberth roberth commented Jan 7, 2020

This can be further remedied by backporting your path.check fix #77133 if necessary.

Infinisil added 2 commits Jan 8, 2020
For upcoming allowance of paths as submodules
@Infinisil Infinisil force-pushed the Infinisil:paths-as-submodules branch from 5b6cdb8 to b6e6d47 Jan 8, 2020
Infinisil added a commit that referenced this pull request Jan 8, 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.

(cherry picked from commit d7a109b)

See #76861 (comment)
for why this is cherry-picked
@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Jan 8, 2020

Removed the allowsPathsAsValues again, fixed the either in certmgr, backported the path.check fix and added a release note for the incompatibility.

@danbst
Copy link
Contributor

@danbst danbst commented Jan 9, 2020

I'd say this is bad. Can't we already use types.submodule { imports = [ ./path-to-options ]; }? If shortcut brings such tricky behavior, maybe its not worth adding it.

@roberth
Copy link
Contributor

@roberth roberth commented Jan 9, 2020

@danbst It's for values, which occur more frequently than option/type declarations. (I think you mistyped?)
The real trickiness is in either, not submodule.
I think it's worth it because it makes the syntax for modules more consistent and therefore easier to understand.

@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Jan 9, 2020

It allows e.g.

some-submodule = mkMerge [
  ./some-module.nix
  (mkIf some-condition ./other-module.nix)
]

instead of

some-submodule = mkMerge [
  { imports = [ ./some-module.nix ]; }
  (mkIf some-condition { imports = [ ./other-module.nix ]; })
]

It's not that big of a benefit, but I don't think we should not allow that just because of a slight inconvenience with the either type in a very special case.

Co-Authored-By: Robert Hensing <roberth@users.noreply.github.com>
@Infinisil Infinisil force-pushed the Infinisil:paths-as-submodules branch from 6a0c941 to 9d4b59b Jan 9, 2020
@danbst
Copy link
Contributor

@danbst danbst commented Jan 9, 2020

@Infinisil I don't think it is possible to mkMerge sub modules. Will it be possible after this change?

nix-repl> (evalModules { 
  modules = [ 
   { 
    options.bla = mkOption { 
        type = with types; submodule (mkMerge [ { options.foobar = mkOption {}; } ]); 
    }; 
   }  
   { bla = {}; } 
  ]; }
).config.bla
error: The option `bla.options' defined in `<unknown-file>' does not exist.

However, if I remove mkMerge [], it works.

@danbst
Copy link
Contributor

@danbst danbst commented Jan 9, 2020

@roberth this also adds an exception, when order of items in either matters. ADT with exceptions :)

I can't build of top of my head a counterexample (when either is "ADT with exceptions" with current types), maybe it is flawed already. In that case, adding another "exception" is fine.

@roberth
Copy link
Contributor

@roberth roberth commented Jan 9, 2020

@danbst as much as I'd like either to be an algebraic sum type, it isn't. It's an untagged union with untagged union problems.

Also, because a mathematical union discards duplicates, if more than one field of the union can take on a single common value, it is impossible to tell from the value alone which field was last written.
-- wikipedia

either should have been called union.
"Either" is commonly used for tagged unions, in Haskell, Scala, Purescript and a bunch of languages and libraries. It should have been avoided.

@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Jan 9, 2020

Oh yeah, what I showed above (the non-path version) doesn't work, it needs to be this (if you don't use submoduleWith) which is even worse:

some-submodule = mkMerge [
  ({ ... }: { imports = [ ./some-module.nix ]; })
  (mkIf some-condition ({ ... }: { imports = [ ./other-module.nix ]; }))
]

Compare that to the version when paths are allowed

@danbst
Copy link
Contributor

@danbst danbst commented Jan 9, 2020

@Infinisil still nope. I've cloned your branched and checked that paths don't work either:

[danbst@station:~/dev/nixpkgs-pr-76861]$ nix eval '(with import ./lib; (evalModules { 
  modules = [ 
   { 
    options.bla = mkOption { 
        type = with types; submodule (mkMerge [ ./dummy.nix ]); 
    }; 
   }  
   { bla = {}; } 
  ]; }
).config.bla
)'
error: value is a path while a set was expected, at /home/danbst/dev/nixpkgs-pr-76861/lib/modules.nix:208:25
(use '--show-trace' to show detailed location information)

And yes, if I remove mkMerge it works.

@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Jan 9, 2020

Ah I didn't pay attention to the code, no that doesn't work because mkMerge isn't supposed to be used for option types, it's only for the config section. Your example without mkMerge already worked previously indeed. This change is only for the cases like the example I showed (assigning submodules in the config section)

@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Jan 9, 2020

Here's a simple full example of what's now possible (Edit: Which is pretty much what the test case does)

config.nix:

{ config, lib, pkgs, ... }: {
  options.some-submodule = lib.mkOption {
    type = lib.types.submodule [];
  };

  config.some-submodule = ./some-module.nix;
}

some-module.nix:

{ lib, ... }: {
  options.foo = lib.mkOption {
    default = false;
  };
}
$ nix-instantiate --eval nixos --arg configuration ./config.nix -A config.some-submodule.foo
false
@danbst
Copy link
Contributor

@danbst danbst commented Jan 9, 2020

@roberth okay, you are right. either (attrsOf str) (attrsOf int) behaves differently depending on order of type arguments....

@danbst
danbst approved these changes Jan 9, 2020
pull bot added a commit to nyanloutre/nixpkgs that referenced this pull request Jan 10, 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.

(cherry picked from commit d7a109b)

See NixOS#76861 (comment)
for why this is cherry-picked
pull bot added a commit to nyanloutre/nixpkgs that referenced this pull request Jan 10, 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.

(cherry picked from commit d7a109b)

See NixOS#76861 (comment)
for why this is cherry-picked
pull bot added a commit to nyanloutre/nixpkgs that referenced this pull request Jan 10, 2020
* cyrus_sasl: add patch for CVE-2019-19906

sourced from debian as patch isn't even in upstream master yet.

(cherry picked from commit 302a77a)

* nspr: 4.23 -> 4.24

* sqlite: 3.30 -> 3.30.1

* nss_3_48: 3.47.1 -> 3.48

* firefox: prepare for firefox 72

* firefox: 71.0 -> 72.0

* firefox-bin: 71.0 -> 72.0

* firefox-beta-bin: 72.0b1 -> 73.0b1

* firefox-devedition-bin: 72.0b1 -> 73.0b1

* photoqt: use qt5's mkDerivation

(cherry picked from commit 787a7f6)

* apache-kafka.nix: Add missing quote inside tmpfiles rule

(cherry picked from commit 39cd457)
Backport of NixOS#75182

* python: Add support for installing Python eggs

(cherry picked from commit 2d6f1ff)

* swiftclient: add setuptools

Traceback (most recent call last):
  File "/nix/store/8qkdlyv2ckrimvi50qvl0anzv66jcp2j-python-swiftclient-3.6.0/bin/.swift-wrapped", line 7, in <module>
    from swiftclient.shell import main
  File "/nix/store/8qkdlyv2ckrimvi50qvl0anzv66jcp2j-python-swiftclient-3.6.0/lib/python3.7/site-packages/swiftclient/__init__.py", line 20, in <module>
    from .client import *  # noqa
  File "/nix/store/8qkdlyv2ckrimvi50qvl0anzv66jcp2j-python-swiftclient-3.6.0/lib/python3.7/site-packages/swiftclient/client.py", line 33, in <module>
    from swiftclient import version as swiftclient_version
  File "/nix/store/8qkdlyv2ckrimvi50qvl0anzv66jcp2j-python-swiftclient-3.6.0/lib/python3.7/site-packages/swiftclient/version.py", line 15, in <module>
    import pkg_resources
ModuleNotFoundError: No module named 'pkg_resources'

(cherry picked from commit dfd115a)

* nix: 2.3.1 -> 2.3.2

(cherry picked from commit 3b15451)

* firefox-esr-68: 68.3.0esr -> 68.4.0esr

(cherry picked from commit 2ad59bd)

* disnix: 0.9 -> 0.9.1

(cherry picked from commit 3f0fee7)

* firefox: 72.0 -> 72.0.1

(cherry picked from commit aab1f2d)

* firefox-esr-68: 68.4.0esr -> 68.4.1esr

(cherry picked from commit 204d32a)

* firefox: fix build of >=72 on aarch64

(cherry picked from commit b4983fe)

* lib/types: Fix path type check

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.

(cherry picked from commit d7a109b)

See NixOS#76861 (comment)
for why this is cherry-picked

* duplicati: 2.0.4.5 -> 2.0.4.23

(cherry picked from commit 6cd31dd)
Signed-off-by: Domen Kožar <domen@dev.si>

* firefox-bin: 72.0 -> 72.0.1 [security] CVE-2019-17026

(cherry picked from commit 0271b2c)

* matrix-synapse: 1.7.3 -> 1.8.0

https://github.com/matrix-org/synapse/releases/tag/v1.8.0
(cherry picked from commit 9d845d4)

* pythonPackages.blaze: fix build

* pythonPackages.odo: disable tests, fix build

Co-authored-by: Robert Scott <github@humanleg.org.uk>
Co-authored-by: markuskowa <markus.kowalewski@gmail.com>
Co-authored-by: Andreas Rammhold <andreas@rammhold.de>
Co-authored-by: Andrew Valencik <valencik@users.noreply.github.com>
Co-authored-by: Vladimír Čunát <v@cunat.cz>
Co-authored-by: clefru <clemens@endorphin.org>
Co-authored-by: Sarah Brofeldt <sbrofeldt@gmail.com>
Co-authored-by: adisbladis <adisbladis@gmail.com>
Co-authored-by: worldofpeace <worldofpeace@protonmail.ch>
Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
Co-authored-by: Sander van der Burg <svanderburg@gmail.com>
Co-authored-by: Silvan Mosberger <contact@infinisil.com>
Co-authored-by: Domen Kožar <domen@enlambda.com>
Co-authored-by: Daniel Frank <github-523@danielfrank.net>
Co-authored-by: Maximilian Bosch <maximilian@mbosch.me>
Co-authored-by: Florian Klink <flokli@flokli.de>
Co-authored-by: Frederik Rietdijk <freddyrietdijk@fridh.nl>
@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Jan 11, 2020

@roberth Good to merge?

@roberth roberth merged commit 9884cb3 into NixOS:master Jan 12, 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:paths-as-submodules branch Jan 12, 2020
arcnmx added a commit to arcnmx/nixpkgs that referenced this pull request Jan 14, 2020
This more intuitively matches `types.either` and allows paths to be
coerced to submodules again, which was inhibited by NixOS#76861
@arcnmx arcnmx mentioned this pull request Jan 14, 2020
0 of 10 tasks complete
dtzWill added a commit to dtzWill/nixpkgs that referenced this pull request Jan 19, 2020
This more intuitively matches `types.either` and allows paths to be
coerced to submodules again, which was inhibited by NixOS#76861

(cherry picked from commit 92b464d)
dtzWill added a commit to dtzWill/nixpkgs that referenced this pull request Jan 19, 2020
Until NixOS#76861 or so is merged

(cherry picked from commit be3f887)
Infinisil added a commit to Infinisil/ofborg that referenced this pull request Mar 15, 2020
Without this, a big part of the lib tests aren't being done, which
previously lead to e.g. NixOS/nixpkgs#76861
And this will also be useful for checked maintainers in NixOS/nixpkgs#82461
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.

None yet

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