Conversation
755aaf4 to
cbdf35e
Compare
ysndr
reviewed
Jun 10, 2025
Comment on lines
-851
to
+852
| old_lockfile: Option<Lockfile>, | ||
| new_lockfile: Lockfile, | ||
| old_lockfile: Box<Option<Lockfile>>, | ||
| new_lockfile: Box<Lockfile>, |
Contributor
There was a problem hiding this comment.
question: did the struct get too large or why do we need to start box'ing them here?
I assume these rust changes are a result of also bumping the rust toolchains which brought new clippy lints?
Contributor
Author
There was a problem hiding this comment.
Yeah clippy complained, I assume because of a newer clippy
This updates Nix to 2.28.2, which required: - Includes to be reworked - Patches that have been upstreamed to be dropped - Using nix-unit as a flake since it doesn't yet support Nix 2.28.2 includes
The ParsedURL url field was dropped in Nix commit f705ce7f9a95bc37df236d88eef1dbd9e2ae5f31, which says to use to_string() instead.
As of Nix commit beed00c04e136e8d685905e4b2b1116ecdf42f63, absPath takes a string_view
InputPath was renamed to InputAttrPath in Nix commit 7dfff58292475d0aed3dc3e98236ba495c45f261
As of Nix commit 7b6622d73317f0cd093cde96b5898b8af07abea7, a NixInt is no longer just a typedef so we have to use .value to extract an integer
We should only be using paths from nix.out, not nix.dev. Removing nix.dev from propagatedBuildInputs reduces our closure size by ~60 MiB. Before this change, on aarch64-linux: ``` $ nix path-info --size --closure-size --human-readable ./result /nix/store/58yi21g394bzy5zdg0jzar7x1kg5cp9x-flox-1.4.3-g59e52f0 82.8 KiB 373.2 MiB ``` After: ``` $ nix path-info --size --closure-size --human-readable ./result /nix/store/nwk9pqbxjb37hcwxjd3l5g18g2ig1lmj-flox-1.4.3-g3bb091d 82.8 KiB 315.9 MiB ```
I increased large-error-threshold for now to silence some lints, but we can fix those if we want as followup
Add a newline in expected test output. The previous behavior was incorrect and logged in NixOS/nix#11991 which has since been fixed.
Updating our flake increases closure size on aarch64-linux from 294.8 MiB to 315.9 MiB. The closure size of Nix from 2.24.12 to 2.28.2 went up from 108.4 to 131.1 MiB. As this is a necessary dependency, increase closure limit size accordingly. It does look like Nix may have introduced some unnecessary dependencies, but if we think those are worth chasing down we can do it in followup.
Bump testing nixpkgs URL to stay in sync with the nixpkgs we use to build Flox. If we don't do this, it will cause build tests to fail, as certain build tests currently depend on the fact that patchShebang will patch using a bash that is in the closure of flox-interpreter. If the nixpkgs used for patchShebang is different than the one used to build flox-interpreter, the build will fail with errors like: ``` ❌ 1 packages found in /nix/store/sg8yy9rf2wi4jakxjdfq7ly220647wqh-mypkg-1.0.2a - not found in /nix/store/4w3fvpfl2yjabdb536ij74zlp8hwwz1l-environment-build-mypkg ```
opt-level > 0 causes hyper to create bad requests because of a rustc bug, so set opt-level to 0 I also left .connection_verbose(true) in our client builder as it was helpful during debugging.
zmitchell
approved these changes
Jun 10, 2025
ysndr
added a commit
that referenced
this pull request
Jul 18, 2025
Originally introduced in <#2789> the patch was merged upstream into master in February via <NixOS/nix#12580>. Assuming it was available via Nix in nixpkgs in late June, we removed the patch in <#3176>, which bumped our nix distribution to v2.28.2. It turns out that the upstream patch was only released via nix **2.29.0**, thus by removing our patch we regressed that behavior. This commit reintroduces the patch, while we use nix 2.28.3.
tomberek
pushed a commit
that referenced
this pull request
Jul 18, 2025
Originally introduced in <#2789> the patch was merged upstream into master in February via <NixOS/nix#12580>. Assuming it was available via Nix in nixpkgs in late June, we removed the patch in <#3176>, which bumped our nix distribution to v2.28.2. It turns out that the upstream patch was only released via nix **2.29.0**, thus by removing our patch we regressed that behavior. This commit reintroduces the patch, while we use nix 2.28.3.
This file contains hidden or 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
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.
Run
nix flake updateand deal with the fallout. See commits for details of resulting changes requiredRelease Notes
We could possibly mention we now ship Nix 2.28.2