bundlerEnv: improve handling of groups #51884
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Suppose I have a Gemfile like this:
source "https://rubygems.org"
gem "actioncable"
gem "websocket-driver", group: :test
The gemset.nix generated by Bundix 2.4.1 will set ActionCable's groups
to [ "default" ], and websocket-driver's to [ "test" ]. This means that
the generated bundlerEnv wouldn't include websocket-driver unless the
test group was included, even though it's required by the default group.
This is arguably a bug in Bundix (websocket-driver's groups should
probably be [ "default" "test" ] or just [ "default" ]), but there's no
reason bundlerEnv should omit dependencies even given such an input --
it won't necessarily come from Bundix, and it would be good for
bundlerEnv to do the right thing.
To fix this, filterGemset is now a recursive function, that adds
dependencies of gems in the group to the filtered gemset until it
stabilises on the gems that match the required groups, and all of their
recursive dependencies.
This wasn't really an issue until the latest minor release of Bundix (2.4), because prior to then Bundix didn't emit group attributes, and so this functionality of bundlerEnv wasn't really used. However, it is now apparent that a better default for bundlerEnv would be to include all gem groups by default, not just the default group. This matches the behavior of Bundler, and makes more sense, because the default group alone isn't necessarily useful for anything -- consider a Rails app with production, development, and test groups. It has the additional benefit of being backwards compatible with how this would have worked before the Bundix update.
"default" isn't really a group, it's more the absence of one. With Bundler, this means that a gem should be installed unconditionally, regardless of which groups are specified. It doesn't really make sense to allow these gems to be omitted from a bundlerEnv.
grahamc
reviewed
Dec 12, 2018
|
lgtm on a light review |
cstrahan
reviewed
Apr 7, 2019
| # nix-repl> converge (x: x / 2) 16 | ||
| # 0 | ||
| converge = f: x: | ||
| if (f x) == x |
There was a problem hiding this comment.
@alyssais
This could be optimized by introducing a let binding, i.e.
converge = f: x:
let f_of_x = f x;
in if f_of_x == x
then x
else converge f f_of_x;
That way f x doesn't have to be evaluated twice.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation for this change
The most recent version of Bundix introduces group attribute generation. This has revealed some deficiencies with how groups are handled by bundlerEnv. These would be addressed by making the following changes:
Suppose I have a Gemfile like this:
The gemset.nix generated by Bundix 2.4.1 will set ActionCable's groups to
[ "default" ], and websocket-driver's to[ "test" ]. This means that the generatedbundlerEnvwouldn't include websocket-driver unless the test group was included, even though it's required by the default group.This is arguably a bug in Bundix ("default" isn't included in groups on transitive dependencies listed explicitly in other groups nix-community/bundix#39) (websocket-driver's groups should probably be
[ "default" "test" ]or just[ "default" ]), but there's no reasonbundlerEnvshould omit dependencies even given such an input -- it won't necessarily come from Bundix, and it would be good forbundlerEnvto do the right thing.To fix this,
filterGemsetis now a recursive function, that adds dependencies of gems in the group to the filtered gemset until it stabilises on the gems that match the required groups, and all of their recursive dependencies.All groups are now included by default. This wasn't really an issue until the latest minor release of Bundix (2.4), because prior to then Bundix didn't emit group attributes, and so this functionality of
bundlerEnvwasn't really used. However, it is now apparent that a better default forbundlerEnvwould be to include all gem groups by default, not just the default group. This matches the behavior of Bundler, and makes more sense, because the default group alone isn't necessarily useful for anything -- consider a Rails app with production, development, and test groups. It has the additional benefit of being backwards compatible with how this would have worked before the Bundix update.Default gems are always included.
defaultisn't really a group, it's more the absence of one. With Bundler, this means that a gem should be installed unconditionally, regardless of which groups are specified. It doesn't really make sense to allow these gems to be omitted from abundlerEnv.Things done
sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)nix path-info -Sbefore and after)