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

shell.nix: keep Nixpkgs pinned, but allow override #7703

Closed
wants to merge 1 commit into from

Conversation

shajra
Copy link

@shajra shajra commented Dec 23, 2019

This refines PR #6213 (commit 8dc9764), which pinned Nixpkgs.

In general, pinning Nixpkgs is a good idea. But there's some problems with
pinning it so hard. It makes it harder to experiment with more modern Nixpkgs
without forking the codebase. In general it's nicer to be current if we can,
because it decreases the likelihood of cache misses and having to pull a ton of
stuff down from Hydra.

So this commit tries to get the best of both worlds. The pinned version is
still there as a default. But I can easily override it to test out later
versions (which we'll want to do periodically anyway to not be perpetually stuck
on an ancient version of Nixpkgs).

Description

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

This refines PR qmk#6213 (commit 8dc9764), which pinned Nixpkgs.

In general, pinning Nixpkgs is a good idea.  But there's some problems with
pinning it so hard.  It makes it harder to experiment with more modern Nixpkgs
without forking the codebase.  In general it's nicer to be current if we can,
because it decreases the likelihood of cache misses and having to pull a ton of
stuff down from Hydra.

So this commit tries to get the best of both worlds.  The pinned version is
still there as a default.  But I can easily override it to test out later
versions (which we'll want to do periodically anyway to not be perpetually stuck
on an ancient version of Nixpkgs).
@shajra
Copy link
Author

shajra commented Dec 23, 2019

My testing was largely to run nix-shell, because my change wasn't really semantic... just restructuring a let-binding and function parameters. Mostly I wanted to make sure there wasn't a syntactic error in the Nix expression.

Also, this was done in a way that should be non-breaking with other people consuming this expression, since I used a default argument for the function.

@zvecr zvecr requested a review from a team December 23, 2019 19:33
@zvecr
Copy link
Member

zvecr commented Dec 23, 2019

@jbaum98 As the original author of the pinning, could you please give this a look!

@jbaum98
Copy link
Contributor

jbaum98 commented Dec 23, 2019

Sounds like a good idea and looks good to me. My only thought is that maybe we want to provide a way to change the base nixkgs but use our overlay. Maybe we can also pull the overlay above the function and add an overlays argument whose default value is just our overlay? That way you can customize nixpkgs and the overlays separately.

That's just a thought though, no need to wait on merging this if no one actually needs that functionality.

@drashna
Copy link
Member

drashna commented Dec 25, 2019

@jbaum98 if it looks good to you, would you mind approving it then?
You may not get the green check mark, but it definitely helps us!

Copy link
Contributor

@jbaum98 jbaum98 left a comment

Choose a reason for hiding this comment

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

Sorry, didn't realize I had the ability to approve things!

@drashna
Copy link
Member

drashna commented Dec 25, 2019

@jbaum98 everyone does. Only collaborators get a green check mark, but anyone can approve.
So for people with relevant experience, it helps us with approval and merging.

@stale
Copy link

stale bot commented Feb 8, 2020

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@tzarc
Copy link
Member

tzarc commented Jul 25, 2020

Got some conflicts. @shajra @jbaum98 is this still a valid PR?

@jbaum98
Copy link
Contributor

jbaum98 commented Jul 26, 2020

Yeah I think so, though in the long run we might want to move to the new nix flakes model, which handles things like pinning and also allows for people to potentially run things in this repository without even cloning it, something like nix run github:qmk/qmk_firmware#flash.

@con-f-use
Copy link

con-f-use commented Mar 22, 2021

This one has a better version of nixpkgs as the default: #12295.
And PR #11181 discusses a totally different approach to python dependencies, that is way better and should be the goto eventually.

@purcell
Copy link
Contributor

purcell commented Mar 22, 2021

Yes, this PR is defunct.

@skullydazed skullydazed closed this Apr 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants