Navigation Menu

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

Jump to bottom

haskellPackages.update-nix-fetchgit fixes #102997

Merged
merged 6 commits into from Nov 7, 2020
Merged

haskellPackages.update-nix-fetchgit fixes #102997

merged 6 commits into from Nov 7, 2020

Conversation

expipiplus1
Copy link
Contributor

Motivation for this change

Necessary fixes for update-nix-fetchgit (a very useful tool for nix dev I think, https://asciinema.org/a/fJesaOF7jGKjYcLtUCsOqrZX6 !)

This was written against update-nix-fetchgit 0.2.3 which has yet to make it onto the haskell-updates branch (should happen next time the versions are bumped though).

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@expipiplus1
Copy link
Contributor Author

I also think that this is a pretty motivating example for #100732

@expipiplus1 expipiplus1 force-pushed the joe-update-nix-fetchgit branch 3 times, most recently from 8db9533 to b6ec634 Compare November 6, 2020 07:16
@expipiplus1 expipiplus1 marked this pull request as ready for review November 6, 2020 14:43
@expipiplus1
Copy link
Contributor Author

This is ready to go now that 0.2.3 is in haskell-updates

@expipiplus1
Copy link
Contributor Author

If I add myself to pkgs/development/haskell-modules/configuration-hackage2nix.yaml as a maintainer for this package will that work considering that @sorki is already a maintainer?

@expipiplus1 expipiplus1 mentioned this pull request Nov 6, 2020
Merged
@peti
Copy link
Member

peti commented Nov 6, 2020

I would prefer if we avoid use of allowInconsistentDependencies in Nixpkgs at all costs. This "feature" is a dangerous hack that's very likely to produce broken binaries.

@expipiplus1
Copy link
Contributor Author

expipiplus1 commented Nov 6, 2020 via email

@peti peti force-pushed the haskell-updates branch 4 times, most recently from a7d0cd6 to d1e684c Compare November 6, 2020 20:38
@expipiplus1
Copy link
Contributor Author

@peti I have removed the use of allowInconsistentDependencies and rebased on haskell-updates.

cdepillabout
cdepillabout reviewed Nov 7, 2020
View reviewed changes
Copy link
Member

@cdepillabout cdepillabout left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good other than the overrideScope thing.

pkgs/development/haskell-modules/configuration-common.nix Outdated Show resolved Hide resolved
pkgs/development/haskell-modules/configuration-common.nix Outdated
tasty-hunit = self.tasty-hunit.override { inherit tasty; };
};
}) (drv: {
buildTools = drv.buildTools or [ ] ++ deps ++ [ pkgs.makeWrapper ];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct? Does update-nix-fetchgit really need git and nix as a buildtool?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're used for the tests. A fake git repo and nix store are created to test updating expressions based on the result of nix-prefetch-git

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're used for the tests.

In that case, I imagine they should probably go in testToolDepends?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah of course, thanks. I've added this now, along with addTestToolDepends to lib.nix

@expipiplus1 expipiplus1 force-pushed the joe-update-nix-fetchgit branch 2 times, most recently from 8715e0a to aad8e23 Compare November 7, 2020 03:24
@expipiplus1
haskell: add addTestToolDepends
43df864
@expipiplus1
haskellPackages.optparse-applicative_0_16_0_0: Fix for compgen error
df7f9d1
@expipiplus1
haskellPackages.optparse-generic_1_4_4: Correct version of dependency
ccacffc
@expipiplus1
haskellPackages.update-nix-fetchgit: Several fixes
4df9d2c 
Fix versions of dependencies

Generate shell completions

Wrap executable so that required executables are in PATH

Make sure necessary executables for tests are present during build
cdepillabout
cdepillabout reviewed Nov 7, 2020
View reviewed changes
pkgs/development/haskell-modules/lib.nix
Comment on lines +159 to +160
addTestToolDepend = drv: x: addTestToolDepends drv [x];
addTestToolDepends = drv: xs: overrideCabal drv (drv: { testToolDepends = (drv.testToolDepends or []) ++ xs; });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks for adding this :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It not being there was the reason I didn't use it!

@cdepillabout
Copy link
Member

Looks good, and this builds on my machine. Thanks!

@cdepillabout cdepillabout merged commit 82d393e into NixOS:haskell-updates Nov 7, 2020
@expipiplus1 expipiplus1 deleted the joe-update-nix-fetchgit branch November 7, 2020 06:32
@expipiplus1
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdepillabout cdepillabout cdepillabout left review comments

Assignees
No one assigned
Labels
6.topic: haskell 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@expipiplus1 @peti @cdepillabout