-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Allow building current code with flake #868
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
base: hovudstraum
Are you sure you want to change the base?
Conversation
|
i'm entirely blank on the nixos stuff, so -- @chinponya @toast003 could I have your opinion on this? also if either of you ever get tired of the pings let me know and I'll stop :p |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this. This actually helps more than it appears at a glance, since it allows us to more easily test builds.
The idea has some issues: any updates that involve changes to web deps could leave us with a broken build. I'd imagine these will be fairly rare and it is called "unstable" for a reason. As a stop-gap solution until we start building web deps with nix, I think it will do just fine.
I'm probably bikeshedding here too much.
| overlays = [ self.overlays.default ] ++ lib.singleton (final: prev: { | ||
| # This is here in flake.nix rather than common/packages/nix because it needs to access the flake metadata to get the date of the most recent commit (lastModifiedDate) | ||
| copyparty-unstable = final.copyparty.overrideAttrs (finalAttrs: previousAttrs: { | ||
| version = "${previousAttrs.version}-unstable-${self.lastModifiedDate}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it is preferred to append the commit hash for untagged versions. In this case self.shortRev would work.
| withZeroMQ = true; | ||
| withFTPS = true; | ||
| withSMB = true; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not caused by this PR but I am now realizing that this list is missing a couple attributes such as withMagic and withTFTP. It might be worth including here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imo that should fixed be in a different pr
| lib = import "${nixpkgs}/lib"; | ||
| inherit (self) lastModifiedDate; | ||
| year = lib.toIntBase10 (builtins.substring 0 4 lastModifiedDate); | ||
| month = lib.toIntBase10 (builtins.substring 4 2 lastModifiedDate); | ||
| day = lib.toIntBase10 (builtins.substring 6 2 lastModifiedDate); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These helpers exist only for the unstable overlay and are defined at the top-level here. Maybe co-locating them with package overrides would be better?
| overlays = [ self.overlays.default ] ++ lib.singleton (final: prev: { | ||
| # This is here in flake.nix rather than common/packages/nix because it needs to access the flake metadata to get the date of the most recent commit (lastModifiedDate) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might need to keep all overlays within the overlay.nix file, in order to make them available for users who do not use flake-enabled setups (and I know they exist here).
Unfortunately I do not have a good recommendation on how to preserve the date/revision once this segment is extracted out of flake.nix.
Maybe we don't actually need it and merely appending unstable to the previous release version is good enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I think just adding unstable (or git) would be good enough
Previously, the flake could only build the pinned (release) version of copyparty. Now it can also build the code in the repo around it, with the exception of webdeps which it copies from the pinned version.
With these modifications the
--versionoutput ofcopyparty-unstablelooks like:This PR complies with the DCO; https://developercertificate.org/